Skip to content

jsdialog based calc tab-bar, removed jquery-contextmenu#15423

Open
dennisfrancis wants to merge 7 commits intomainfrom
private/dennisf/jsdialog-ctxtmenu
Open

jsdialog based calc tab-bar, removed jquery-contextmenu#15423
dennisfrancis wants to merge 7 commits intomainfrom
private/dennisf/jsdialog-ctxtmenu

Conversation

@dennisfrancis
Copy link
Copy Markdown
Member

@dennisfrancis dennisfrancis commented Apr 8, 2026

  • Target version: main

Summary

  • jsdialog based calc tab-bar context menu.
  • Change in calc cypress test helper.
  • Remove unused Control.ContextMenu.js

This is related to #13948

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@Darshan-upadhyay1110
Copy link
Copy Markdown
Contributor

Hi @dennisfrancis! Is this PR ready for review? If so, could you add the appropriate reviewer(s)? Thanks! 🙂

@dennisfrancis dennisfrancis marked this pull request as draft April 15, 2026 14:10
@dennisfrancis
Copy link
Copy Markdown
Member Author

Hi @dennisfrancis! Is this PR ready for review? If so, could you add the appropriate reviewer(s)? Thanks! 🙂

This still needs some work. I've marked this as draft. Thanks.

@dennisfrancis dennisfrancis force-pushed the private/dennisf/jsdialog-ctxtmenu branch from 5f21ff9 to 3717806 Compare April 15, 2026 17:53
@dennisfrancis dennisfrancis marked this pull request as ready for review April 15, 2026 17:53
@dennisfrancis
Copy link
Copy Markdown
Member Author

Screenshots:

  • Before
image
  • After
image

@dennisfrancis dennisfrancis force-pushed the private/dennisf/jsdialog-ctxtmenu branch from 3717806 to 807bb3c Compare April 15, 2026 19:42
@dennisfrancis dennisfrancis requested a review from eszkadev April 15, 2026 19:51
@eszkadev
Copy link
Copy Markdown
Contributor

please rebase to pass tests

@dennisfrancis dennisfrancis force-pushed the private/dennisf/jsdialog-ctxtmenu branch from 807bb3c to 3e05872 Compare April 16, 2026 15:33
@dennisfrancis
Copy link
Copy Markdown
Member Author

CI fails in impress mobile:

Test failed: integration_tests/mobile/impress/slide_properties_spec.js / Changing slide properties. / Change master objects visibility.
                    
                    Timed out retrying after 10000ms: expected true to be false
                    
                    /home/collabora/jenkins/workspace/github_online_master_debug_vs_co-26.04_cypress_mobile/cypress_test/integration_tests/common/helper.js:647:34
                      645 |             expect(result).to.be.true;
                      646 |         else
                    > 647 |             expect(result).to.be.false;
                          |                                  ^
                      648 |     });
                      649 |     cy.log('<< isImageWhite - end');
                      650 | }

But I can't reproduce this with latest online main branch.

@dennisfrancis dennisfrancis force-pushed the private/dennisf/jsdialog-ctxtmenu branch from 3e05872 to 3d9dfbd Compare April 17, 2026 12:35
@eszkadev eszkadev force-pushed the private/dennisf/jsdialog-ctxtmenu branch 2 times, most recently from 2653686 to bfe7fb7 Compare April 17, 2026 16:19
eszkadev
eszkadev previously approved these changes Apr 17, 2026
Copy link
Copy Markdown
Contributor

@eszkadev eszkadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked well for me, I removed also the library itself.
Adding second reviewer

@github-project-automation github-project-automation bot moved this from To Review to To Test in Collabora Online Apr 17, 2026
@eszkadev eszkadev requested a review from timar April 17, 2026 16:20
@eszkadev eszkadev changed the title jsdialog based calc tab-bar context menu jsdialog based calc tab-bar, removed jquery-contextmenu Apr 17, 2026
dennisfrancis and others added 7 commits April 19, 2026 18:16
This is related to #13948

Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: Ie8b1b0e12946d3a1d87f0104ed174623f068ea89
Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: Id63728812ce4142a4549fbdedfcf9bde85859c79
Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: I668c711dbac98cead4fe2ed3715906b484387f76
Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: I85462dd7acf2c129dcc402b69dbc068a26772316
Signed-off-by: Dennis Francis <dennis.francis@collabora.com>
Change-Id: I0dca0b9272935946df30f9684cc5b48feb02b03f
- we no longer use jQuery menu so no need to close it
- do not need main level CSS for context menu

Signed-off-by: Szymon Kłos <szymon.klos@collabora.com>
Change-Id: If7fdc0af34af7d9eb72f7089437ee02f568a34d3
Signed-off-by: Szymon Kłos <szymon.klos@collabora.com>
Change-Id: I43cea6c46a1bd30324c3dc77fa5a8434e01095a1
@dennisfrancis dennisfrancis force-pushed the private/dennisf/jsdialog-ctxtmenu branch from bfe7fb7 to 5b1b288 Compare April 19, 2026 12:48
@dennisfrancis
Copy link
Copy Markdown
Member Author

@eszkadev The removal of css in css/jsdialogs.css and/or css/device-mobile.css has caused cypress mobile failure in sheet operations. When running make run-mobile spec=calc/sheet_operation_spec.js it shows oversized menu entries. Excluding the those removals fixes the cypress issue.

@eszkadev
Copy link
Copy Markdown
Contributor

Interesting, I did grep and didn't found these classes. Must be concatenated somewhere. Thanks for fixing


const isUNO = key.startsWith('.uno:');
if (Object.prototype.hasOwnProperty.call(data, '_uno')) key = data._uno;
let text = isUNO ? _UNO(key, 'spreadsheet', true): _(data._text);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_( ) requires string inside, you cannot do it this way,
Please do it in the JSON

isHtmlName: true,
callback: (this._insertSheetBefore).bind(this)
callback: (this._insertSheetBefore).bind(this),
_text: 'Insert sheet before this',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need _( ) here to get it working

isHtmlName: true,
callback: (this._insertSheetAfter).bind(this)
callback: (this._insertSheetAfter).bind(this),
_text: 'Insert sheet after this',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_()

callback: (this._moveSheetLeft).bind(this),
visible: areTabsMultiple
visible: areTabsMultiple,
_text: 'Move Sheet Left',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_()

callback: (this._moveSheetRight).bind(this),
visible: areTabsMultiple
visible: areTabsMultiple,
_text: 'Move Sheet Right',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_()

return !areTabsMultiple();
}
},
_text: 'Copy Sheet...',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_()

@eszkadev eszkadev dismissed their stale review April 20, 2026 05:02

translations need a fix

Copy link
Copy Markdown
Member

@timar timar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SBOM changes look OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To Test

Development

Successfully merging this pull request may close these issues.

4 participants