Fix Navbar responsive controls for shadow DOM hosts#1539
Open
Fix Navbar responsive controls for shadow DOM hosts#1539
Conversation
Navbar.showMobileControls and showDesktopControls used
`document.querySelector('.BRnavMain .controls ...')` and
`document.querySelector('.BRnavMobile')` to toggle the `.hide` class
on view-mode buttons at different viewport widths. Those lookups
cannot cross shadow DOM boundaries — so when BookReader is embedded
inside a shadow-DOM host (e.g. archive.org's offshoot details page),
every query returns null, no class gets toggled, and the mobile-only
viewmode button stays visible alongside the three desktop view-mode
buttons (one-page, two-page, thumbnail). Users see a duplicate
thumbnail icon in the navbar.
Scope the queries to `this.$nav` (the navbar's own jQuery-wrapped
root) instead. $nav is a wrapped set with `.BRnavMain` and
`.BRnavMobile` as top-level sibling elements, so the outer classes
must be matched with `.filter()` — `.find()` only traverses
descendants and would silently match nothing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1539 +/- ##
==========================================
+ Coverage 68.98% 69.00% +0.01%
==========================================
Files 65 65
Lines 5556 5552 -4
Branches 1229 1223 -6
==========================================
- Hits 3833 3831 -2
+ Misses 1689 1687 -2
Partials 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
Navbar.showMobileControls/showDesktopControlsto work when BookReader is hosted inside a shadow-DOM ancestor (e.g. archive.org's offshoot details page, or any web-component host).Problem
Both methods use
document.querySelectorto find the view-mode buttons and the mobile-nav wrapper:document.querySelectorcan't cross shadow DOM boundaries. When BookReader renders inside a shadow-scoped ancestor, every lookup returns null, no.hideclass is ever toggled, and both sets of controls render simultaneously — the mobile-onlyviewmodebutton shows next to the three desktop view-mode buttons (onepg,twopg,thumb), producing a duplicate thumbnail icon in the navbar at desktop widths.Fix
Scope the queries to
this.$nav(the navbar's own jQuery-wrapped root element) instead ofdocument.$navis built from a fragment containing two top-level sibling divs (.BRnavMobile+.BRnavMain), so:.find('.controls .${name}')scoped inside.filter('.BRnavMain')for the inner control buttons.filter('.BRnavMobile')for the outer mobile nav (not.find('.BRnavMobile'), which is descendants-only and would silently miss the top-level sibling)This makes both methods shadow-DOM-agnostic while preserving identical behavior in the light-DOM case.
Verification
showDesktopControls/showMobileControls, and asserts the expected.hideclass transitions.Risk
Low. The jQuery
.filter()/.find()chain produces the same DOM set that thedocument.querySelectorversions were reaching in the document-case, and the method surface is unchanged.Related: addresses the same class of shadow-DOM bug as #1520.