Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

browsersetting bug fixes #11042

Merged
merged 5 commits into from
Feb 4, 2025
Merged

browsersetting bug fixes #11042

merged 5 commits into from
Feb 4, 2025

Conversation

Rash419
Copy link
Contributor

@Rash419 Rash419 commented Jan 29, 2025 •
edited
Loading

No description provided.

@Rash419 Rash419 requested a review from caolanm January 29, 2025 08:57
@caolanm caolanm enabled auto-merge (rebase) January 29, 2025 09:14
@Rash419 Rash419 disabled auto-merge January 30, 2025 14:18
@Rash419 Rash419 changed the title ui: when using browsersetting return default value if key is not present browsersetting bug fixes Jan 30, 2025
@Rash419
Copy link
Contributor Author

Rash419 commented Jan 30, 2025 •
edited
Loading

I think cypress tests are failing due to my patch. I need to investigate

@Rash419 Rash419 force-pushed the preset-browser-setting-bug-fix branch 2 times, most recently from 20b7f7f to 5acb5c8 Compare February 3, 2025 07:13
@Rash419 Rash419 requested a review from caolanm February 3, 2025 07:51
@Rash419
Copy link
Contributor Author

Rash419 commented Feb 4, 2025 •
edited
Loading

only one cypress test calc/top_toolbar_spec.js -> Save is failing with:

     cy:command ✘  uncaught exception	TypeError: Cannot read properties of null (reading 'hasChildNodes')
      cy:command ✔  error:	Cannot read properties of null (reading 'hasChildNodes')
                    TypeError: Cannot read properties of null (reading 'hasChildNodes')
                        at Object.removeChildNodes (http://localhost:9900/browser/5acb5c8e07/src/dom/DomUtil.js:92:19)
                        at NewClass._onRefresh (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1566:19)
                        at NewClass._onDocLayerInit (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1654:14)
                        at NewClass._onRefresh (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1563:18)
                        at DocEvents.fire (http://localhost:9900/browser/5acb5c8e07/src/app/DocEvents.js:14:24)
                        at NewClass._enterEditMode (http://localhost:9900/browser/5acb5c8e07/src/control/Permission.js:191:20)
                        at NewClass.setPermission (http:// ...
      cy:command ✘  error:	Cannot read properties of null (reading 'hasChildNodes')
                    TypeError: Cannot read properties of null (reading 'hasChildNodes')
                        at Object.removeChildNodes (http://localhost:9900/browser/5acb5c8e07/src/dom/DomUtil.js:92:19)
                        at NewClass._onRefresh (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1566:19)
                        at NewClass._onDocLayerInit (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1654:14)
                        at NewClass._onRefresh (http://localhost:9900/browser/5acb5c8e07/src/control/Control.Menubar.js:1563:18)
                        at DocEvents.fire (http://localhost:9900/browser/5acb5c8e07/src/app/DocEvents.js:14:24)
                        at NewClass._enterEditMode (http://localhost:9900/browser/5acb5c8e07/src/control/Permission.js:191:20)
                        at NewClass.setPermission (http:// ...

@Rash419
ui: when using browsersetting return default value if key is not present
33f0176 
- this fixes a weird bug where sometimes there is darkBackground in
light theme

Signed-off-by: Rashesh <[email protected]>
Change-Id: I9587e7b717bd65d1ff1360b9f1b43ac12133af3f
@Rash419
js: fix: browsersetting: compactMode not initialized correctly
b72a484 
- The menubar and top toolbar need to be reinitialized if `useBrowserSetting`
is enabled. Initially, `initializeBasicUI` is called before the browsersetting
are received, meaning we can't determine if `compactMode` is enabled.
- To address this, we perform another check before `doclayerinit` and add the
necessary UI controls if the conditions are met.

Signed-off-by: Rashesh <[email protected]>
Change-Id: I8d53486200fdfd49501e821334b56bc1f5b9a93d
@Rash419
wsd: browsersetting: override accessibilityState
b05c511 
- needed because client doesn't know about the `accessibilityState`
  preference when sending load url message

Signed-off-by: Rashesh <[email protected]>
Change-Id: I965b255bebe83d3c943842559742e6d8bcef56e8
@Rash419 Rash419 force-pushed the preset-browser-setting-bug-fix branch from 5acb5c8 to e495a2a Compare February 4, 2025 16:44
@Rash419 Rash419 enabled auto-merge (rebase) February 4, 2025 16:54
@Rash419 Rash419 force-pushed the preset-browser-setting-bug-fix branch from e495a2a to b33d83a Compare February 4, 2025 20:11
@Rash419
js: fix null exception in _onRefresh due to missing event removal
3995fec 
Fixes an issue where _onRefresh in Control.Menubar.js throws a null
exception because the updatepermission event listener was not removed
when removeClassicUI was called.

Issue:
1. The client initializes basicUI, but at this stage, the browsersetting
message has not been received yet.
2. Since useBrowserSetting is false, it may fallback to localStorage,
which could have "compactMode=true", leading to the initialization of
the menubar.
3. Later, when the browsersetting JSON is received, it determines the
final UI mode. If notebookbar mode is preferred, the menubar and top
toolbar are removed.
4. However, while removing events, the updatepermission event was not
detached. As a result, when updatepermission fired, _onRefresh was still
called on a now-null _map, causing an exception.

Fix:
- Ensure that the updatepermission event listener is properly removed
when removeClassicModeUI is executed.

Signed-off-by: Rashesh <[email protected]>
Change-Id: Ic5dc86c6b5350670e586966226ba722135a55b0d
@Rash419
cypress: fix: switchUICompact after reloading the document
7077448 
- when `browsersetting` is enabled we don't store compactMode preference
to localStorage

Signed-off-by: Rashesh <[email protected]>
Change-Id: Ie5bb5be7abf923b7530d854a8489f5f5c97858c3
@Rash419 Rash419 force-pushed the preset-browser-setting-bug-fix branch from b33d83a to 7077448 Compare February 4, 2025 20:39
@Rash419 Rash419 merged commit bdc6af7 into CollaboraOnline:master Feb 4, 2025
12 of 13 checks passed
@Rash419 Rash419 deleted the preset-browser-setting-bug-fix branch February 4, 2025 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caolanm caolanm caolanm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Collabora Online
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rash419 @caolanm