1402094 - stylo: tools in the Reader Mode appear to have double borders.

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[bbell]

Attachment: example.png

Recently the look of the tools on the Reader Mode changed. They appear to be beveled when no bevel should be there.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Probably web-observable regression.



Regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4c86474c75be02a4d568a33bce49d31bbbf88fa5&tochange=6d8d80beb8c82d1617b9fe939fe6035df3044738

Doesn't reproduce when turning off stylo. I expect it's the css-parser update. Manish?

Comment 2

[:Gijs (he/him)]

Err, surprise plot twist...

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fa4de7ce4de0d9d5fcb4baffa600e439992b9e4b&tochange=18723c6944b1d45e0044e100a8fdc8f573df4714

Bobby/Xidorn? Can't see that bug... Still suspect it is, at its core, a stylo issue that is just being exposed because maybe about:reader was getting non-stylo before and is now getting stylo? In any case, that seems likely given that flipping the pref makes it go away.

Comment 3

[:Gijs (he/him)]

Web console produces:

13:41:30.181 Unknown pseudo-class or pseudo-element ‘scope’.  Ruleset ignored due to bad selector.
narrateControls.css:1:1

So this basically looks like bug 1377144 being regressed by bug 1400540. Don't know if that's intentional or not.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Gijs from comment #2)
> Bobby/Xidorn? Can't see that bug... Still suspect it is, at its core, a
> stylo issue that is just being exposed because maybe about:reader was
> getting non-stylo before and is now getting stylo? In any case, that seems
> likely given that flipping the pref makes it go away.

Yeah, before that change, about:reader was getting non-stylo because it is about: and not about:blank.

The patch in bug 1400540 makes any non-system non-xul document use stylo, so if about:reader doesn't use system principal, and it isn't xul document, it gets stylo.

It's not clear to me what problem is it in the document, though. Stylo doesn't support scoped style element, but it isn't immediately clear to me why that becomes a problem for about:reader in this case. It seems there is still some behavior difference between stylo and non-stylo which we probably should fix?

If it turns out that this is because of scope, we can probably whitelist about:reader to use non-stylo as well for now, as far as content script cannot control about:reader in any sense...

Comment 5

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> (In reply to :Gijs from comment #2)
> > Bobby/Xidorn? Can't see that bug... Still suspect it is, at its core, a
> > stylo issue that is just being exposed because maybe about:reader was
> > getting non-stylo before and is now getting stylo? In any case, that seems
> > likely given that flipping the pref makes it go away.
> 
> Yeah, before that change, about:reader was getting non-stylo because it is
> about: and not about:blank.
> 
> The patch in bug 1400540 makes any non-system non-xul document use stylo, so
> if about:reader doesn't use system principal, and it isn't xul document, it
> gets stylo.
> 
> It's not clear to me what problem is it in the document, though. Stylo
> doesn't support scoped style element, but it isn't immediately clear to me
> why that becomes a problem for about:reader in this case. It seems there is
> still some behavior difference between stylo and non-stylo which we probably
> should fix?

I don't think so, I think this is only the :scope selector and other <style scoped> behaviour. The :scope selector sets a CSS variable for the border which is then used later on, so right now those rules will be broken because the variable is not defined, leading to the obviously broken appearance.

Unfortunately, I think breaking <style scoped> for reader has other, worse effects on android (per bug 1377144).

> If it turns out that this is because of scope, we can probably whitelist
> about:reader to use non-stylo as well for now, as far as content script
> cannot control about:reader in any sense...

I think this is the safest option for 57. For 58, I can look at no longer using <style scoped> on about:reader, though it's not entirely trivial.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

That would be very appreciated, because it doesn't seem to me stylo is getting scoped style support soonish, but we really want to remove the original style system as soon as possible.

Comment 7

[Bobby Holley (:bholley)]

Ok. Xidorn, can you whitelist reader mode for 57? Gijs, can you find an owner for removing style scoped from reader mode in 58?

Comment 8

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> Gijs, can you find an
> owner for removing style scoped from reader mode in 58?

I think this will probably have to be me, unless dolske has a better idea?

Effectively, to fix this "properly", we probably need to fix bug 1204818. A wallpaper would be updating all the styles with descendant selectors such that they can't easily "accidentally" be used by content that happens to have the same ids/classes.

Comment 9

[Justin Dolske [:Dolske]]

I'm ok with having Gijs be on the hook for this for now (either to fix it or help someone else do so). Gijs -- let's chat offline about the scope (ha) of work here?

Is 58 actually the required target for this? We're going be fixing polish bugs on 57-Beta for a while still, which makes the 58-Nightly cycle short.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Justin Dolske [:Dolske] from comment #9)
> I'm ok with having Gijs be on the hook for this for now (either to fix it or
> help someone else do so). Gijs -- let's chat offline about the scope (ha) of
> work here?

Great, thanks!

> Is 58 actually the required target for this? We're going be fixing polish
> bugs on 57-Beta for a while still, which makes the 58-Nightly cycle short.

We have a handful of blockers for dropping the old style system, and it's hard to say how long the longest pole is going to be. We are similarly preoccupied with 57, so it would be reasonable to check in a few weeks down the line to see where everybody's at.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1402094 - Blacklist about:reader from using stylo.

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

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8913057 [details]
Bug 1402094 - Blacklist about:reader from using stylo.

https://reviewboard.mozilla.org/r/184454/#review189608

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ce27eeef766
Blacklist about:reader from using stylo. r=bholley

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage on Android: dom/base/nsDocument.cpp:13613: 'bool ShouldUseGeckoBackend(nsIURI*)' defined but not used:

https://hg.mozilla.org/integration/autoland/rev/c70dcd8228e28af2c100340559accb48e7adee72

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8ce27eeef7662c04deed1328ff25937e530aaf79&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133748878&repo=autoland

[task 2017-09-28T06:47:08.500Z] 06:47:08     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base5.cpp:128:0:
[task 2017-09-28T06:47:08.500Z] 06:47:08     INFO -  /builds/worker/workspace/build/src/dom/base/nsDocument.cpp: At global scope:
[task 2017-09-28T06:47:08.500Z] 06:47:08     INFO -  /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:13613:1: error: 'bool ShouldUseGeckoBackend(nsIURI*)' defined but not used [-Werror=unused-function]
[task 2017-09-28T06:47:08.500Z] 06:47:08     INFO -   ShouldUseGeckoBackend(nsIURI* aDocumentURI)
[task 2017-09-28T06:47:08.500Z] 06:47:08     INFO -   ^

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8913057 [details]
Bug 1402094 - Blacklist about:reader from using stylo.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/813556d9e389
Blacklist about:reader from using stylo. r=bholley

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/813556d9e389

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8913057 [details]
Bug 1402094 - Blacklist about:reader from using stylo.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1400540
[User impact if declined]: may see some visual deficit in reader mode
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: open reader mode and have a look at the sidebar
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it just makes reader mode use gecko style backend rather than stylo
[String changes made/needed]: n/a

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913057 [details]
Bug 1402094 - Blacklist about:reader from using stylo.

OK, let's use the old mode instead of stylo until we are ready.
Should be in 57b5

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35bae846b629

Comment 21

[roxleitan]

Verified fixed using the latest Nightly (2017-10-12) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.

Comment 22

[Ciprian Georgiu, Desktop QA]

Reproduced this bug using an affected Nightly build from 2017-09-21.

Verified fixed on 57 Beta 13 (20171030163911) as well across platforms: Win 10 x64, macOS 10.13 and Ubuntu 16.04 x64.