Closed Bug 1377144 Opened 2 years ago Closed 2 years ago

Reader Mode: text-colored dot in the bottom right of the screen.

Categories

(Firefox for Android :: Reader View, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
fennec + ---
firefox54 --- unaffected
firefox55 + verified
firefox56 + fixed

People

(Reporter: nbp, Assigned: heycam)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

With Firefox for Android nightly (56.0a1 2017-06-28), I am able to reproduce an issue with 2 different websites:
 - about:reader?url=http%3A%2F%2Fmashable.france24.com%2Ftech-business%2F20170627-monnaie-virtuelle-ethereum-consommation-electricite
 - about:reader?url=http%3A%2F%2Fdussutou.free.fr%2Fpublication_audrey_dussutour.html

The issue appear as a text-colored dot, at the bottom right of the screen, and it does not move while scrolling.  It appears both in portrait and landscape views.
I see the same thing on two different devices, using nytimes.com and washingtonpost.com.
I reported this in bug 1376419.
Duplicate of this bug: 1376419
Duplicate of this bug: 1377986
I do like me my ReaderMode ....

This seems to regress from Bug 1291515:
https://hg.mozilla.org/mozilla-central/rev/9ed72cb74889

Re-setting |pref("layout.css.scoped-style.enabled", true);| works as a work around for now.
Regression window performed:
Last good revision: d50abca6521baeae8ac6b07ddf843d63a1aa5f84 (2017-06-26)
First bad revision: 8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b (2017-06-27)
This bug just showed up in beta 55.0-2015498601. beta 55.0-2015496769 is ok. The config fix works for the new beta.
Blocks: 1291515
[Tracking Requested - why for this release]: Reader mode config button broken on Android.
tracking-fennec: --- → ?
OS: Unspecified → Android
Hardware: Unspecified → All
Duplicate of this bug: 1379361
Duplicate of this bug: 1379439
Hi Tim, could you find someone to look at this?  I'm not sure where the code is that uses <style scoped> (the disabling of which got uplifted to beta) to add the in-page UI for reader mode.
Flags: needinfo?(timdream)
Look like we'll need to partially revert bug 1154028.

http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/reader/content/aboutReader.html#16

Gijs, assuming bug 1154028 comment 1 is still correct, this can be fixed by prepending class names both content and controls on all selectors right? Please suggest any alternatives if you have any.
Flags: needinfo?(timdream) → needinfo?(gijskruitbosch+bugs)
If we want a quick fix, we could just allow <style scoped> in about:reader?xxx pages for now.  (Stylo won't be enabled for about: pages other than about:blank initially, so that won't affect us.)
regression from a recent uplift to beta55, tracking.
FWIW unless there's a low-risk fix for reader mode in the next few hours I'll probably ask for part 2 of bug 1291515 to be backed out of beta so the regression is fixed in today's build.
Comment 13 is pretty low risk.  Let me work that one up.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8884737 - Flags: review?(xidorn+moz)
The patch is untested since I don't have a local environment set up for building Fennec.  I've pushed a try run so hopefully I can grab a build from that to test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=520beb2d9b0a4436aa6465cb3ad4e6a018042628
Attachment #8884737 - Flags: review?(xidorn+moz) → review?(emilio+bugs)
Comment on attachment 8884737 [details]
Bug 1377144 - Allow <style scoped> in about:reader documents.

https://reviewboard.mozilla.org/r/155620/#review160618

::: commit-message-39f79:1
(Diff revision 1)
> +Bug 1377144 - Allow <scoped style> in about:reader documents.

nit: `<style scoped>`.

::: dom/base/nsDocument.cpp:13485
(Diff revision 1)
>    mIsThirdParty.emplace(false);
>    return mIsThirdParty.value();
>  }
>  
> +static bool
> +IsAboutReader(nsIURI* aURI)

nit: Perhaps worth leaving a comment about why we have to whitelist about:reader URIs?
Attachment #8884737 - Flags: review?(emilio+bugs) → review+
:maliu confirmed that the patch fixes the regression.
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d48958204f3
Allow <style scoped> in about:reader documents. r=emilio
Comment on attachment 8884737 [details]
Bug 1377144 - Allow <style scoped> in about:reader documents.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1291515
[User impact if declined]: potential odd styling in Reader Mode on Android
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: OK:

  1. Open this bug's URL field.
  2. Notice a circle that shouldn't be present (as in this bug's screen shot attachment), if the fix didn't work.  If the fix did work no circle should be present.

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it does a simple URL check on the document and just re-enables <style scoped> for about:reader documents
[String changes made/needed]: none
Attachment #8884737 - Flags: approval-mozilla-beta?
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> Look like we'll need to partially revert bug 1154028.
> 
> http://searchfox.org/mozilla-central/rev/
> b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/reader/content/
> aboutReader.html#16
> 
> Gijs, assuming bug 1154028 comment 1 is still correct, this can be fixed by
> prepending class names both content and controls on all selectors right?
> Please suggest any alternatives if you have any.

It wouldn't really be a complete fix (unless we go overboard with child selectors to fix depth-from-document-root) because we know that we aren't currently perfect at stripping out styles and classes from readerized web content. That's not the end of the world, but really it points to the fact that the correct solution to both this and several other reader mode problems we have is to sandbox-iframe the readerized content. This is bug 1204818. It's not going to be trivial to do, though srcdoc should make it easier now than it used to be and so it shouldn't be rocket science, either.

If we can go with a stopgap fix (which it looks like we're doing) for now and then prioritize the sandboxing bug that would be the best way forward, IMO.
Comment on attachment 8884737 [details]
Bug 1377144 - Allow <style scoped> in about:reader documents.

fix a regression in fennec reader mode introduced in 55.0b7

Should be in 55.0b8
Attachment #8884737 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: nobody → cam
https://hg.mozilla.org/mozilla-central/rev/3d48958204f3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Duplicate of this bug: 1379871
Verified as fixed on Beta 55.0b8 (2017-11-07).
Devices:
-HTC Nexus 9 (Android 7.1.1)
-Prestigio Grace X5 (Android 4.4.2)
-HTC 10 (Android 6.0.1)
-LG G4 (Android 5.1)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → +
Depends on: 1402094
You need to log in before you can comment on or make changes to this bug.