Closed Bug 1417837 Opened 7 years ago Closed 7 years ago

de-scope the style sheets in aboutReader.html

Categories

(Toolkit :: Reader Mode, defect, P1)

Product:
Toolkit
Component:
Reader Mode
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(3 files)

Bug 1417837 - Part 1: De-scope about:reader style sheets.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1417837 - Part 2: Preserve class names in readerized output that we use in aboutReader.css.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1417837 - Part 3: Replace all IDs in about:reader documents with class names.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
<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.
Attachment #8930411 - Flags: review?(gijskruitbosch+bugs)
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 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 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 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)
(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)
Blocks: 1406274
Assignee: nobody → cam
Depends on: 1421757
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.
Attachment #8930411 - Flags: review?(gijskruitbosch+bugs) → 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

> 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
Flags: needinfo?(cam)
Sorry, I fixed those ESlint failures locally but forgot to re-push updated patches.
Flags: needinfo?(cam)
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
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.
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
No longer blocks: 1345702
Blocks: 1444905
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: