<style scoped> is going away. To ensure that rules from aboutReaderControls.css don't accidentally apply to the readerized content, Gijs suggests changing Readability.js to remove any class="" and id="" attributes from the output that the script didn't add itself. Submitted https://github.com/mozilla/readability/pull/406 for that, and in this bug I'll attach a patch to de-scope the reader mode style sheets.

Comment on attachment 8929289 [details] Bug 1417837 - Part 1: De-scope about:reader style sheets. https://reviewboard.mozilla.org/r/200604/#review206840 Ship it! (Though this reminds me I need to take a broom through some of this stuff at some point...)

Comment on attachment 8930341 [details] Bug 1417837 - Part 2: Preserve class names in readerized output that we use in aboutReader.css. https://reviewboard.mozilla.org/r/201476/#review206844

Comment on attachment 8930411 [details] Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names. https://reviewboard.mozilla.org/r/201582/#review206846 I think this is the previous version of the Readability changes? :-) ::: toolkit/components/reader/Readability.js (Diff revision 1) > - * DO NOT MODIFY THIS FILE DIRECTLY! > - * > - * This is a shared library that is maintained in an external repo: > - * https://github.com/mozilla/readability > - */ > - > -/* Please revert this hunk before landing. :-)

Comment on attachment 8930411 [details] Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names. https://reviewboard.mozilla.org/r/201582/#review207170

So it turns out we do have some tests that fail after stripping out element IDs, like browser_readerMode_with_anchor.js. That seems to be checking that we do indeed scroll to the element in the URL fragment. So I think that means we don't want to strip out IDs at all. Does that sound like the right thing to do? We could have an option to specify which IDs to strip, rather than which to preserve, so that we could list all of the IDs we use in aboutReader.css to ensure they don't match. Or we could re-work the rules a little so that the IDs are all prefixed with some class name (e.g. use ".container #reader-message" instead of just "#reader-message"). WDYT Gijs?

(In reply to Cameron McCormack (:heycam) from comment #17) > So it turns out we do have some tests that fail after stripping out element > IDs, like browser_readerMode_with_anchor.js. That seems to be checking that > we do indeed scroll to the element in the URL fragment. So I think that > means we don't want to strip out IDs at all. *smacks forehead* Sorry for not thinking of that before now, either... > Does that sound like the right > thing to do? Yeah, that makes sense. > We could have an option to specify which IDs to strip, rather than which to > preserve, so that we could list all of the IDs we use in aboutReader.css to > ensure they don't match. Or we could re-work the rules a little so that the > IDs are all prefixed with some class name (e.g. use ".container > #reader-message" instead of just "#reader-message"). > > WDYT Gijs? Hum. Probably easiest to adapt the aboutReader.css files so they don't rely on ids alone? Though I don't think a descendant selector would work for all of those because that would still match the items inside the reader content if they're in the same container... Perhaps we should just switch the style stuff to use classes instead of ids anyway? We control the aboutReader.html markup, too...

Gijs, can you look at the part 1 patch again? I came across the narrator style sheets, which I missed the first time around. I decided to remove all the IDs from the document, and work just with classes. Well, there is one place I couldn't remove an ID: https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/toolkit/components/narrate/VoiceSelect.jsm#18-21 since it's used as the target of the aria-controls="" attribute.

Comment on attachment 8929289 [details] Bug 1417837 - Part 1: De-scope about:reader style sheets. https://reviewboard.mozilla.org/r/200604/#review209820 Yep, this still looks good to me - thanks for catching the narrate thing, I should have noticed that in review. :-(

Comment on attachment 8930411 [details] Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names. https://reviewboard.mozilla.org/r/201582/#review209822 Looks good to me, though I would personally have used `querySelector` over `getElementsByClassName`. Was there a particular reason you went for the latter? ::: mobile/android/themes/core/aboutReader.css:39 (Diff revision 3) > > body.serif { > font-family: serif; > } > > -#container.font-size1 { > +.container.font-size1 { Can't remember if I said this already, but seems like we could simplify this with a CSS variable and/or calc() these days. Follow-up fodder. :-) ::: toolkit/components/narrate/NarrateControls.jsm:259 (Diff revision 3) > - let dropdown = this._doc.getElementById("narrate-dropdown"); > + let dropdown = this._doc.getElementsByClassName("narrate-dropdown")[0]; > dropdown.classList.toggle("keep-open", speaking); > dropdown.classList.toggle("speaking", speaking); > > - let startStopButton = this._doc.getElementById("narrate-start-stop"); > + let startStopButton = this._doc.getElementsByClassName("narrate-start-stop")[0]; > startStopButton.title = > gStrings.GetStringFromName(speaking ? "stop" : "start"); > > - this._doc.getElementById("narrate-skip-previous").disabled = !speaking; > - this._doc.getElementById("narrate-skip-next").disabled = !speaking; > + this._doc.getElementsByClassName("narrate-skip-previous")[0].disabled = !speaking; > + this._doc.getElementsByClassName("narrate-skip-next")[0].disabled = !speaking; Was there a particular reason not to use `.querySelector` here instead? That seems neater for not needing the [0] bit here. Could even, potentially, use some other container than `this._doc` to search for these elements.

Comment on attachment 8930411 [details] Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names. https://reviewboard.mozilla.org/r/201582/#review209822 > Was there a particular reason not to use `.querySelector` here instead? That seems neater for not needing the [0] bit here. Could even, potentially, use some other container than `this._doc` to search for these elements. I was under the mistaken impression that getElementsByTagName() would be faster, but I was confusing nsIDocument's name table (fast lookup for elements by name="") with one for classes. I'll switch to querySelector.

Comment on attachment 8930411 [details] Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names. https://reviewboard.mozilla.org/r/201582/#review209822 > Can't remember if I said this already, but seems like we could simplify this with a CSS variable and/or calc() these days. Follow-up fodder. :-) Filed bug 1422680 for someone to look at.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa4f488ea88f Part 1: De-scope about:reader style sheets. r=Gijs https://hg.mozilla.org/integration/autoland/rev/1c341a427a7a Part 2: Preserve class names in readerized output that we use in aboutReader.css. r=Gijs https://hg.mozilla.org/integration/autoland/rev/0d6b56293cbf Part 3: Replace all IDs in about:reader documents with class names. r=Gijs

Backed out 3 changesets (bug 1417837) for ESlint failures at /builds/worker/checkouts/gecko/toolkit/components/narrate/NarrateControls.jsm r=backout on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0d6b56293cbfe68325158535c2305fe7f3930ae1 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=149483159&repo=autoland&lineNumber=206 Backout: https://hg.mozilla.org/integration/autoland/rev/0c3e9673b64539d05949bd932bb8c74835a0dc73

Sorry, I fixed those ESlint failures locally but forgot to re-push updated patches.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a76f2363663 Part 1: De-scope about:reader style sheets. r=Gijs https://hg.mozilla.org/integration/autoland/rev/49a463e85e42 Part 2: Preserve class names in readerized output that we use in aboutReader.css. r=Gijs https://hg.mozilla.org/integration/autoland/rev/9556f12b4058 Part 3: Replace all IDs in about:reader documents with class names. r=Gijs

Backed out 3 changesets (bug 1417837) for failing browser/chrome/test_media_playback.html r=backout on a CLOSED TREE https://treeherder.mozilla.org/logviewer.html#?job_id=149500752&repo=autoland&lineNumber=2194 https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9556f12b4058df88153ff984a75f5af792d1bf58&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable https://hg.mozilla.org/integration/autoland/rev/b00fbc8e4554c7e3bed4981cb1bd652b459684c5

I don't *think* this needs to make 58, but leaving fix-optional rather than wontfix in case there's a need/strong desire to uplift this from a stylo perspective.

(In reply to Dorel Luca [:dluca] from comment #35) > Backed out 3 changesets (bug 1417837) for failing > browser/chrome/test_media_playback.html r=backout on a CLOSED TREE I'm not sure that test failure is related to these patches, but definitely one following it (Android-specific reader moder test) is.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f1c76eceff4b Part 1: De-scope about:reader style sheets. r=Gijs https://hg.mozilla.org/integration/autoland/rev/4f2678770830 Part 2: Preserve class names in readerized output that we use in aboutReader.css. r=Gijs https://hg.mozilla.org/integration/autoland/rev/f151e6be30b8 Part 3: Replace all IDs in about:reader documents with class names. r=Gijs

https://hg.mozilla.org/mozilla-central/rev/f1c76eceff4b https://hg.mozilla.org/mozilla-central/rev/4f2678770830 https://hg.mozilla.org/mozilla-central/rev/f151e6be30b8