de-scope the style sheets in aboutReader.html

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
Reader Mode
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fix-optional, firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

<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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8930411 - Flags: review?(gijskruitbosch+bugs)

Comment 9

2 months ago
mozreview-review
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...)
Attachment #8929289 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 10

2 months ago
mozreview-review
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
Attachment #8930341 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

2 months ago
mozreview-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/#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. :-)
Attachment #8930411 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
mozreview-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/#review207170
Attachment #8930411 - Flags: review?(cam) → review+
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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

2 months ago
(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...
Flags: needinfo?(gijskruitbosch+bugs)

Updated

2 months ago
Assignee: nobody → cam

Updated

2 months ago
Depends on: 1421757
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 23

2 months ago
mozreview-review
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 24

2 months ago
mozreview-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.
Attachment #8930411 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 25

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 months ago
mozreview-review-reply
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.

Comment 30

2 months ago
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
Flags: needinfo?(cam)
Sorry, I fixed those ESlint failures locally but forgot to re-push updated patches.
Comment hidden (mozreview-request)
Flags: needinfo?(cam)

Comment 34

2 months ago
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

Comment 36

2 months ago
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.
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
status-firefox59: --- → affected
Priority: -- → P1
(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.
Comment hidden (mozreview-request)

Comment 40

2 months ago
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

Comment 41

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1c76eceff4b
https://hg.mozilla.org/mozilla-central/rev/4f2678770830
https://hg.mozilla.org/mozilla-central/rev/f151e6be30b8
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.