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)

defect
Not set
normal

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)
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)
Flags: needinfo?(gijskruitbosch+bugs)
(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)
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
Hi Gijs,

I've added the test. Could you help review it? Thanks.
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-
Hi Gijs,

Updated the patch for review comments. Please review it again. Thanks. :)
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+
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
Flags: needinfo?(gijskruitbosch+bugs)
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/
Does Bug 1313278 cause this failure mentioned on Comment 13? I'm not sure.
Bug 1342348 addressed the hash link issue.
Depends on: 1342348
(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)
> 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)
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`.
Flags: needinfo?(gijskruitbosch+bugs)
Thanks for reviewing, Gijs.
Keywords: checkin-needed
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
Tomcat,

Could you help land the patch into m-c? Thanks.
Flags: needinfo?(cbook)
(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)
https://hg.mozilla.org/mozilla-central/rev/362a5654bf6b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
> 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.