Closed
Bug 1340799
Opened 7 years ago
Closed 7 years ago
Add an automated test for clicking hash links in reader mode scrolling to the anchor (rather than exiting reader mode)
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: Gijs, Assigned: evanxd)
References
Details
Attachments
(2 files)
In bug 1280888 we fixed some code such that links in reader mode point to the correct page. That code might be broken by bug 1313278 and friends. It would be useful to have an automated test for anchor links working correctly given that they are widespread in technical websites and should work normally in reader mode. Evan, do you think you could work on this?
Flags: needinfo?(evan)
Assignee | ||
Comment 1•7 years ago
|
||
Sure, let me work on this. But currently if we follow the STR you mentioned in [1], the actual result is not correct, it doesn't open in a new tab but it does nothing(doesn't scroll anywhere). I can reproduce the issue with the Nightly Firefox built from current m-c code base in the video[2]. ``` 1. open http://www.gutenberg.org/files/3300/3300-h/3300-h.htm 2. enter reader mode 3. click a link in the table of contents ``` I think we should fix the issue first then add this tests. What do you think? [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1280888#c0 [2]: https://www.youtube.com/watch?v=x3MmAbdNZ3g
Flags: needinfo?(evan)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Evan Tseng [:evanxd] from comment #1) > I think we should fix the issue first then add this tests. What do you think? See bug 1280888 comment 21. We should still be able to test this even if that particular testcase fails.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 3•7 years ago
|
||
Assigning per comment #1. I think we should work on this ASAP given that it looks like bug 1313278 has landed on Nightly.
Assignee: nobody → evan
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Hi Gijs, I've added the test. Could you help review it? Thanks.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8840321 [details] Bug 1340799 - Add a test for clicking hash links in reader mode to scroll to the anchor, https://reviewboard.mozilla.org/r/114826/#review116330 ::: toolkit/components/reader/test/browser_readerMode_with_anchor.js:29 (Diff revision 2) > + yield BrowserTestUtils.withNewTab(TEST_PATH + "readerModeArticle.html", function* (browser) { > + let pageShownPromise = BrowserTestUtils.waitForContentEvent(browser, "AboutReaderContentReady"); > + let readerButton = document.getElementById("reader-mode-button"); > + readerButton.click(); > + yield pageShownPromise; > + EventUtils.synthesizeMouseAtCenter(browser.contentDocument.getElementById("foo-anchor"), {}, browser.contentWindow); This should use a selector to execute the click with the e10s machinery of BrowserTestUtils.synthesizeMouse, rather than a CPOW. You will need to yield for that. ::: toolkit/components/reader/test/browser_readerMode_with_anchor.js:32 (Diff revision 2) > + readerButton.click(); > + yield pageShownPromise; > + EventUtils.synthesizeMouseAtCenter(browser.contentDocument.getElementById("foo-anchor"), {}, browser.contentWindow); > + yield ContentTask.spawn(browser, null, function* () { > + // Check if offset != 0 > + ok(content.document.getElementById("foo") !== null, "foo element should be in document"); No point strictly comparing with null, just ok(content.document.getElementById(), ...) will do. ::: toolkit/components/reader/test/browser_readerMode_with_anchor.js:33 (Diff revision 2) > + yield pageShownPromise; > + EventUtils.synthesizeMouseAtCenter(browser.contentDocument.getElementById("foo-anchor"), {}, browser.contentWindow); > + yield ContentTask.spawn(browser, null, function* () { > + // Check if offset != 0 > + ok(content.document.getElementById("foo") !== null, "foo element should be in document"); > + ok(content.pageYOffset != 0, "pageYOffset should be > 0"); I would prefer to actually check that foo has been scrolled into view. If that's somehow not possible, at a minimum this should be using isnot(content.pageYOffset, 0, ...) rather than ok(), and there should be a check before we navigate to the hash link that the page offset is 0 at that point.
Attachment #8840321 -
Flags: review?(gijskruitbosch+bugs) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Hi Gijs, Updated the patch for review comments. Please review it again. Thanks. :)
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8840321 [details] Bug 1340799 - Add a test for clicking hash links in reader mode to scroll to the anchor, https://reviewboard.mozilla.org/r/114826/#review116440 r=me if this is actually green on try.
Attachment #8840321 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Hi Gijs, I updated patch to fix the test failure[1]. ``` TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode_with_anchor.js | page should be scrolled to the foo anchor - 2881 == 2882 - ``` The failure(`is(content.document.documentElement.scrollTop, foo.offsetTop)`) only occurred on Linux but it is OK on Mac OS. I think let's do `isnot(content.pageYOffset, 0)` here. Then file a followup issue to address it. The second issue of the patch is the test we added in the patch cannot be passed for the current m-c build because the `href` attribute of a hash link will be remove in reader mode, like the attachment screenshot. Do you know which commit we need to be back out, or something? [1]: https://treeherder.mozilla.org/logviewer.html#?job_id=79722823&repo=try&lineNumber=1963
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 14•7 years ago
|
||
If the change[1] is OK, let merge it after we back out the commit caused the failure(or do other things to fix the failure) and the CI is green. [1]: https://reviewboard.mozilla.org/r/114826/diff/5/
Assignee | ||
Comment 15•7 years ago
|
||
Does Bug 1313278 cause this failure mentioned on Comment 13? I'm not sure.
Assignee | ||
Comment 16•7 years ago
|
||
Bug 1342348 addressed the hash link issue.
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Evan Tseng [:evanxd] (Away 2/25 - 2/28) from comment #13) > Created attachment 8840774 [details] > Current m-c build remove href attribute of a hash link > > Hi Gijs, > > I updated patch to fix the test failure[1]. > ``` > TEST-UNEXPECTED-FAIL | > toolkit/components/reader/test/browser_readerMode_with_anchor.js | page > should be scrolled to the foo anchor - 2881 == 2882 - > ``` > The failure(`is(content.document.documentElement.scrollTop, foo.offsetTop)`) > only occurred on Linux but it is OK on Mac OS. I think let's do > `isnot(content.pageYOffset, 0)` here. Then file a followup issue to address > it. Could we just do something like checking the absolute difference between the two is smaller than or equal to 1? That seems like it'd work, e.g.: let {scrollTop} = content.document.documentElement; let {offsetTop} = foo; Assert.lessOrEqual(Math.abs(scrollTop - offsetTop), 1, `scrollTop (${scrollTop}) should be within 1 CSS pixel of offsetTop (${offsetTop})`);
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
> Could we just do something like checking the absolute difference between the
> two is smaller than or equal to 1? That seems like it'd work, e.g.:
>
> let {scrollTop} = content.document.documentElement;
> let {offsetTop} = foo;
> Assert.lessOrEqual(Math.abs(scrollTop - offsetTop), 1, `scrollTop
> (${scrollTop}) should be within 1 CSS pixel of offsetTop (${offsetTop})`);
Sure, let's do it. Please take a look. If the patch is good now, please help land it. Thanks.
One more question, do you think that is an issue, the `content.document.documentElement.scrollTop` doesn't equal the `foo.offsetTop` on Linux? If it is, I think we should file a followup bug.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8840321 [details] Bug 1340799 - Add a test for clicking hash links in reader mode to scroll to the anchor, https://reviewboard.mozilla.org/r/114826/#review117762 One more nit. The dep has landed, so I think with this fixed you can just mark this reviewboard issue fixed and then set checkin-needed? ::: toolkit/components/reader/test/browser_readerMode_with_anchor.js:38 (Diff revision 7) > + yield BrowserTestUtils.synthesizeMouseAtCenter("#foo-anchor", {}, browser); > + yield ContentTask.spawn(browser, null, function* () { > + let foo = content.document.getElementById("foo"); > + let {scrollTop} = content.document.documentElement; > + let {offsetTop} = foo; > + ok(foo, "foo element should be in document"); This should come before the destructuring assignment to `offsetTop`.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/362a5654bf6b Add a test for clicking hash links in reader mode to scroll to the anchor, r=Gijs
Keywords: checkin-needed
Assignee | ||
Comment 25•7 years ago
|
||
Tomcat, Could you help land the patch into m-c? Thanks.
Flags: needinfo?(cbook)
Reporter | ||
Comment 26•7 years ago
|
||
(In reply to Evan Tseng [:evanxd] from comment #25) > Tomcat, > > Could you help land the patch into m-c? Thanks. The patch doesn't need any special "help" to land on m-c. If it's in autoland, it'll eventually hit m-c when autoland next gets merged.
Flags: needinfo?(cbook)
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/362a5654bf6b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 28•7 years ago
|
||
> The patch doesn't need any special "help" to land on m-c. If it's in
> autoland, it'll eventually hit m-c when autoland next gets merged.
Learned. Thanks, Gijs.
You need to log in
before you can comment on or make changes to this bug.
Description
•