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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: text-colored-dot-bottom-right.png

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.

Comment 1

[Marcia Knous [:marcia]]

I see the same thing on two different devices, using nytimes.com and washingtonpost.com.

Comment 2

[Ken Riley]

I reported this in bug 1376419.

Comment 5

[Mark Capella [:capella]]

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.

Comment 6

[Oana Horvath [:ohorvath]]

Regression window performed:
Last good revision: d50abca6521baeae8ac6b07ddf843d63a1aa5f84 (2017-06-26)
First bad revision: 8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b (2017-06-27)

Comment 7

[Ken Riley]

This bug just showed up in beta 55.0-2015498601. beta 55.0-2015496769 is ok. The config fix works for the new beta.

Comment 8

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]: Reader mode config button broken on Android.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 13

[Cameron McCormack (:heycam)]

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.)

Comment 14

[Julien Cristau [:jcristau]]

regression from a recent uplift to beta55, tracking.

Comment 15

[Julien Cristau [:jcristau]]

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 16

[Cameron McCormack (:heycam)]

Comment 13 is pretty low risk.  Let me work that one up.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Bug 1377144 - Allow <style scoped> in about:reader documents.

Review commit: https://reviewboard.mozilla.org/r/155620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155620/

Comment 18

[Cameron McCormack (:heycam)]

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

Comment 19

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8884737 [details]
Bug 1377144 - Allow <style scoped> in about:reader documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155620/diff/1-2/

Comment 21

[Cameron McCormack (:heycam)]

:maliu confirmed that the patch fixes the regression.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d48958204f3
Allow <style scoped> in about:reader documents. r=emilio

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[:Gijs (he/him)]

(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 25

[Julien Cristau [:jcristau]]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/735095797d9e

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d48958204f3

Comment 29

[Oana Horvath [:ohorvath]]

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)