Closed
Bug 1160775
Opened 9 years ago
Closed 9 years ago
Reader view button does not appear during automated tests for readerModeArticle.html
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | + | fixed |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
People
(Reporter: Margaret, Assigned: Gijs)
References
Details
Attachments
(1 file, 1 obsolete file)
See bug 1158228 comment 5. I disabled the visibility helper function on fx-team, so that we could land the Readability.js changes, since this requires some more investigation.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 1•9 years ago
|
||
:-( The visibility check checks the size of an element without flushing layout. This must be breaking somehow though it didn't in my testing. We'll need to figure out why, or accept that the first time we call it, we *should* flush layout, maybe? Not sure offhand. Margaret, if checking manually with the visibility test enabled, do you see issues with the reader mode button not showing up?
Flags: needinfo?(margaret.leibovic)
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #1) > :-( > > The visibility check checks the size of an element without flushing layout. > This must be breaking somehow though it didn't in my testing. We'll need to > figure out why, or accept that the first time we call it, we *should* flush > layout, maybe? Not sure offhand. > > Margaret, if checking manually with the visibility test enabled, do you see > issues with the reader mode button not showing up? Yes, when I loaded this test page with a build with the visibility check, I didn't see the button appear: http://people.mozilla.org/~mleibovic/tmp/readerModeArticle.html But when I attached the debugger before loading that page, I *did* see the button appear.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #2) > (In reply to :Gijs Kruitbosch from comment #1) > > :-( > > > > The visibility check checks the size of an element without flushing layout. > > This must be breaking somehow though it didn't in my testing. We'll need to > > figure out why, or accept that the first time we call it, we *should* flush > > layout, maybe? Not sure offhand. > > > > Margaret, if checking manually with the visibility test enabled, do you see > > issues with the reader mode button not showing up? > > Yes, when I loaded this test page with a build with the visibility check, I > didn't see the button appear: > http://people.mozilla.org/~mleibovic/tmp/readerModeArticle.html > > But when I attached the debugger before loading that page, I *did* see the > button appear. I don't see it appear on beta, period...
Assignee | ||
Comment 4•9 years ago
|
||
/r/8239 - Bug 1160775 - fix reader mode detection to force 1 flush so we don't think the entire page is invisible, r?margaret Pull down this commit: hg pull -r 14995b33eb0b0e39e5c0df86a2c9020a2953e192 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602190 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca97b042650d
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 2
Flags: qe-verify-
Flags: needinfo?(gijskruitbosch+bugs)
Flags: in-testsuite+
Flags: firefox-backlog+
Reporter | ||
Comment 6•9 years ago
|
||
https://reviewboard.mozilla.org/r/8239/#review6987 Ship It!
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8602190 [details] MozReview Request: bz://1160775/Gijs https://reviewboard.mozilla.org/r/8237/#review6989 Ship It!
Attachment #8602190 -
Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/9d1cf0bd3626
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8602190 [details]
MozReview Request: bz://1160775/Gijs
Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: reader mode doesn't deal well with invisible content
[Describe test coverage new/current, TreeHerder]: has tests!
[Risks and why]: fairly low, considering things initially failed tests and now pass, we're pretty confident this is the correct fix.
[String/UUID change made/needed]: nope
Attachment #8602190 -
Flags: approval-mozilla-release?
Attachment #8602190 -
Flags: approval-mozilla-beta?
Attachment #8602190 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
Comment on attachment 8602190 [details]
MozReview Request: bz://1160775/Gijs
Approved for uplift to beta 39. This doesn't need uplift to aurora since it already landed in 40.
Attachment #8602190 -
Flags: approval-mozilla-beta?
Attachment #8602190 -
Flags: approval-mozilla-beta+
Attachment #8602190 -
Flags: approval-mozilla-aurora?
Comment 12•9 years ago
|
||
Comment on attachment 8602190 [details]
MozReview Request: bz://1160775/Gijs
I reviewed with gijs on irc. In addition to the automated test he did verify manually with the patch. This looks good for 38.0.5b2.
Attachment #8602190 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•9 years ago
|
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
tracking-firefox38.0.5:
--- → +
Comment 15•9 years ago
|
||
I think we need to back this out because this breaks normal web browsing experience. (Bug 1172270) Seems to happen regularly on several Finnish discussion forums (or at least the symptoms are exactly the same as in Bug 1172270), though, there the issue happens often even without reload. (But shift+reload is anyway like the initial page load) And the patch looks wrong anyway. We shouldn't force flushing during page load if just possible - that makes page load slower, in some cases significantly slower. Looks like the relevant method is called for example in DOMContentLoaded and pageshow event listeners.
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #15) > I think we need to back this out because this breaks normal web browsing > experience. (Bug 1172270) > Seems to happen regularly on several Finnish discussion forums (or at least > the symptoms are exactly the same as in Bug 1172270), though, there the > issue happens often even without reload. > (But shift+reload is anyway like the initial page load) > > And the patch looks wrong anyway. We shouldn't force flushing during page > load if just possible - that makes page load slower, in some cases > significantly slower. > Looks like the relevant method is called for example in DOMContentLoaded and > pageshow event listeners. I expect this happens on pages where there are no scripts that cause DCL to be delayed until after stylesheets are loaded, meaning DCL fires and we cause a flush then, before stylesheets have loaded. Can you suggest a workaround? We really don't want to wait for "load" because on many more frequently used pages (large publication/news websites, for instance) that takes minutes because of all the crap they load. Even DCL is barely usable for this purpose. We don't seem to have a useful event for "when we show most of the page on screen" or whatever.
Flags: needinfo?(bugs)
Comment 17•9 years ago
|
||
Seems like we use MozAfterPaint in other cases - so, could we in pageshow and or DOMContentLoaded add listener for MozAfterPaint if utils.getBoundsWithoutFlushing(node); return size 0x0, and then the listener would do the new Readability(uri, doc).isProbablyReaderable(this.isNodeVisible.bind(this, utils)); or similar stuff when size is larger than 0x0 (and remove the listener). also, remove the listener at latest when it has been called once and document's readyState is READYSTATE_COMPLETE. (this way the listener would be called once if added during pageshow)
Flags: needinfo?(bugs)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8602190 -
Attachment is obsolete: true
Attachment #8620221 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•