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

VERIFIED FIXED in Firefox 57

Status

()

P2
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: bbell, Assigned: xidorn)

Tracking

({regression})

57 Branch
mozilla58
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified, firefox58 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted image example.png
Recently the look of the tools on the Reader Mode changed. They appear to be beveled when no bevel should be there.
(Reporter)

Updated

2 years ago
Summary: Border the tools in the Reader Mode appear to have double borders. → Border of tools in the Reader Mode appear to have double borders.

Comment 1

2 years ago
[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?
status-firefox57: --- → affected
status-firefox58: --- → affected
tracking-firefox57: --- → ?
Component: General → Untriaged
Flags: needinfo?(manishearth)
Product: Firefox → Core
Summary: Border of tools in the Reader Mode appear to have double borders. → [Stylo] tools in the Reader Mode appear to have double borders.

Comment 2

2 years ago
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.
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(manishearth)
Flags: needinfo?(bobbyholley)

Comment 3

2 years ago
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.
Blocks: 1377144
tracking-firefox57: ? → ---
Keywords: regression
(Assignee)

Comment 4

2 years ago
(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

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
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.
Ok. Xidorn, can you whitelist reader mode for 57? Gijs, can you find an owner for removing style scoped from reader mode in 58?
Assignee: nobody → xidorn+moz
Flags: needinfo?(bobbyholley) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P2
Summary: [Stylo] tools in the Reader Mode appear to have double borders. → stylo: tools in the Reader Mode appear to have double borders.

Comment 8

2 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.
Flags: needinfo?(dolske)
(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 hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8913057 [details]
Bug 1402094 - Blacklist about:reader from using stylo.

https://reviewboard.mozilla.org/r/184454/#review189608
Attachment #8913057 - Flags: review?(bobbyholley) → review+

Comment 13

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ce27eeef766
Blacklist about:reader from using stylo. r=bholley
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(xidorn+moz)

Comment 16

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/813556d9e389
Blacklist about:reader from using stylo. r=bholley

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/813556d9e389
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Updated

2 years ago
Blocks: 1400540
(Assignee)

Comment 18

2 years ago
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
Attachment #8913057 - Flags: approval-mozilla-beta?
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Component: Untriaged → CSS Parsing and Computation
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
Attachment #8913057 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 20

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/35bae846b629
status-firefox57: affected → fixed
Flags: qe-verify+

Updated

2 years ago
Depends on: 1404611
Verified fixed using the latest Nightly (2017-10-12) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
status-firefox58: fixed → verified
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.