1189491 - Reader view doesn't take anchors into account

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[afranke]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150708165822

Steps to reproduce:

Open a link with an anchor such as http://gitolite.com/gitolite/gitolite.html#basics and switch to reader mode.


Actual results:

Top of the page is displayed.


Expected results:

Page is scrolled down to the anchor position.

Comment 1

[Sébastien Blin [:sblin] [:amarok]]

Hi, I'd like to work on this bug.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #1)
> Hi, I'd like to work on this bug.

Great! Do you know where to begin or do you need some help finding where you should make these changes in the source code?

You can fix this bug by working with an artifact build (https://developer.mozilla.org/en-US/docs/Artifact_builds). You can get help with getting your build compiling by joining the #introduction channel on IRC.

Comment 3

[Sébastien Blin [:sblin] [:amarok]]

Yeah, I think the bug is located somewhere in /toolkit/components/reader

I know how to use ./mach test and already have the link about artifact build :)

Comment 4

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_1.patch

Hi !

I just started to work on this bug. And I've got 2 questions :

+ First, I don't see any test directory for the readerview. Does a test folder exist for this part?
+ The reporter give a link (http://gitolite.com/gitolite/gitolite.html#basics) but in the reader mode, the title Basics is not present.

Note: you can test with http://gitolite.com/gitolite/gitolite.html#your-server (we can see the "Your server" title in the reader view)

Have a nice day,

Comment 5

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #4)
> Created attachment 8804971 [details] [diff] [review]
> 1189491_1.patch
> 
> Hi !
> 
> I just started to work on this bug. And I've got 2 questions :
> 
> + First, I don't see any test directory for the readerview. Does a test
> folder exist for this part?

There is no separate test folder, but it would be a good idea to have one. There are already some tests that could be moved. You can do this by:

- creating a 'test' folder underneath toolkit/components/reader/
- modifying toolkit/components/reader/moz.build to have:

BROWSER_CHROME_MANIFESTS += [
  'test/browser.ini'
]

- creating a browser.ini file in it and move the sections (ie [] and any subsequent ini key-value pairs) from 

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser.ini

for:

[browser_readerMode.js]
support-files =
  readerModeArticle.html
  readerModeArticleHiddenNodes.html
[browser_readerMode_hidden_nodes.js]
support-files =
  readerModeArticleHiddenNodes.html
[browser_bug1124271_readerModePinnedTab.js]
support-files =
  readerModeArticle.html


as well as moving all the files mentioned in the above tidbit from browser/base/content/test/general/ to the new test/ dir you create.

In theory, the tests should then be runnable after a ./mach build faster, using ./mach mochitest toolkit/components/reader/ .

In practice, you might need to add a 

[DEFAULT]
support-files = head.js

section and add a head.js file with a copy of browser/base/content/test/general/head.js's promiseTabLoadEvent and promiseWaitForCondition functions. And finally all 3 of these tests seem to define

const TEST_PATH = "http://example.com/browser/browser/base/content/test/general/";

which will need to be:

const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com");

(which will be resistant to future changes in location for these tests).


... after that, I *think* the tests should run. You can then add your own test by creating a test file & section in the browser.ini file for it.

> + The reporter give a link
> (http://gitolite.com/gitolite/gitolite.html#basics) but in the reader mode,
> the title Basics is not present.

You don't need to fix that here, I will file a separate bug for it.

> Note: you can test with
> http://gitolite.com/gitolite/gitolite.html#your-server (we can see the "Your
> server" title in the reader view)

Great.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8804971 [details] [diff] [review]
1189491_1.patch

Review of attachment 8804971 [details] [diff] [review]:
-----------------------------------------------------------------

Jared's been moving house, so reviewing this as well... I'm sorry about the formatting nits, we're updating our code to use eslint more heavily so it takes care of that kind of thing for you, but it's slow going.

For next time, could you either use mozreview or update your hg options so you generate patches with more lines of context? ./mach mercurial-setup can help with the latter.

::: toolkit/components/reader/AboutReader.jsm
@@ +800,5 @@
>  
>      this._doc.dispatchEvent(
>        new this._win.CustomEvent("AboutReaderContentReady", { bubbles: true, cancelable: false }));
> +
> +    this._goToReference(articleUri.ref);

I think we should do this before we send the custom event.

@@ +988,5 @@
> +
> +  /*
> +   * Scroll reader view to a reference
> +   */
> +  _goToReference: function(ref) {

Nit: we use ES6, so you can use:

 _goToReference(ref) {

@@ +989,5 @@
> +  /*
> +   * Scroll reader view to a reference
> +   */
> +  _goToReference: function(ref) {
> +    let anchors = this._contentElement.querySelectorAll("[id]");

This won't get <a name="foo">Anchor</a> .


It also feels like we're doing a lot of manual work. Does setting the location.href to location.href + "#" + ref not work?

@@ +993,5 @@
> +    let anchors = this._contentElement.querySelectorAll("[id]");
> +    let pos = 0;
> +    for(let anchor of anchors)  {
> +      let anchorId = anchor.getAttribute("id");
> +      if(anchorId === ref) {

Nits if my above suggestion doesn't work:
- spaces after for/if
- let anchorId = anchor.id || anchor.name;
- for string comparisons like this, == is sufficient - no coercion is happening anyway, as the qSA call already guarantees that all items will have an ID or name attribute.

@@ +998,5 @@
> +        pos = anchor.offsetTop;
> +        break;
> +      }
> +    }
> +    this._win.scrollTo(0, pos);

(again, with the caveat to look at this if and only if the URL method doesn't work)

The x coordinate won't work if the item is zoomed, and you're flushing layout by reading offsetTop.

Instead, you could have the loop save the anchor reference, and use:

desiredAnchor.scrollIntoView()

you could even pass in the optional parameters to make this happen smoothly. :-)

Comment 7

[Sébastien Blin [:sblin] [:amarok]]

(In reply to :Gijs Kruitbosch from comment #5
> There are already some tests that could be moved. You can do this by:
> 
> - creating a 'test' folder underneath toolkit/components/reader/
> (...)
> after that, I *think* the tests should run. You can then add your own
> test by creating a test file & section in the browser.ini file for it.

Awesome! it works (I just have to copy more functions in head.js). I just have a problem. The test browser_bug1124271_readerModePinnedTab.js is blocked and wait until I click on the reader button, I don't think it's normal ?

(In reply to :Gijs Kruitbosch from comment #6)
> Jared's been moving house, so reviewing this as well... I'm sorry about the
> formatting nits, we're updating our code to use eslint more heavily so it
> takes care of that kind of thing for you, but it's slow going.
No problem.
 
> For next time, could you either use mozreview or update your hg options so
> you generate patches with more lines of context? 
Yeap


> It also feels like we're doing a lot of manual work. Does setting the
> location.href to location.href + "#" + ref not work?

location.href already contains #ref (%23ref to be more precise). I think we can't do that because the content is added after.

> Instead, you could have the loop save the anchor reference, and use:
> 
> desiredAnchor.scrollIntoView()
Erf, my bad

> Nits if my above suggestion doesn't work:

I just ran a ./mach eslint 


So, I must:
+ Search why browser_bug1124271_readerModePinnedTab.js block
+ Add a test for this bug

Comment 8

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_1.patch

Comment 9

[Sébastien Blin [:sblin] [:amarok]]

MMh I will replace the r= mention in the next patch

Comment 10

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #5
> > There are already some tests that could be moved. You can do this by:
> > 
> > - creating a 'test' folder underneath toolkit/components/reader/
> > (...)
> > after that, I *think* the tests should run. You can then add your own
> > test by creating a test file & section in the browser.ini file for it.
> 
> Awesome! it works (I just have to copy more functions in head.js). I just
> have a problem. The test browser_bug1124271_readerModePinnedTab.js is
> blocked and wait until I click on the reader button, I don't think it's
> normal ?

Err, nope. Any errors in the browser console? You can use:

info("mystring")

to do printf-style debugging of where it's getting stuck, in case that helps. If you prefer a proper debugger, you can add a debugger; keyword in the test and use the --jsdebugger argument with the ./mach mochitest invocation to fire up the debugger before the test runs. It'll break on the debugger keyword, and then you can hopefully step through or set other breakpoints. Note that stepping through statements like

yield promise

is non-intuitive (like, the debugger has no idea about Task.jsm so it'll get lost... you usually want a breakpoint on the next line and then just hit continue).

> (In reply to :Gijs Kruitbosch from comment #6)
> > It also feels like we're doing a lot of manual work. Does setting the
> > location.href to location.href + "#" + ref not work?
> 
> location.href already contains #ref (%23ref to be more precise). I think we
> can't do that because the content is added after.

I'll review properly on Monday, but...

the URL for the page is:

about:reader?url=<escaped-url-of-original-page>

So I would expect %23ref to not actually be reflected in location.hash or the page offset.

Basically what I'm saying is, we should set location.hash (d'oh, not sure why I suggested location.href which is more work) in the same place where you're scrolling to the content right now. I *think* that should work (assuming a matching named/id'd anchor is present, of course).

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> I'll review properly on Monday, but...
> 
> the URL for the page is:
> 
> about:reader?url=<escaped-url-of-original-page>
> 
> So I would expect %23ref to not actually be reflected in location.hash or
> the page offset.
> 
> Basically what I'm saying is, we should set location.hash (d'oh, not sure
> why I suggested location.href which is more work) in the same place where
> you're scrolling to the content right now. I *think* that should work
> (assuming a matching named/id'd anchor is present, of course).

... so then the full URL becomes:

about:reader?url=<escaped-url-of-original-page-with%23ref>#ref

if that makes sense. :-)

Comment 12

[:Gijs (he/him)]

Comment on attachment 8805811 [details] [diff] [review]
1189491_1.patch

Review of attachment 8805811 [details] [diff] [review]:
-----------------------------------------------------------------

For the actual file moves, if you use something like:

hg move --after browser/base/content/test/general/browser_readerMode.js toolkit/components/reader/test/browser_readerMode.js

(and so forth for the other files you moved)

then we don't break blame/annotate for these files. (I think git does this kind of thing automatically - hg has explicit move/copy markers, AIUI.)

This will make the patch a lot smaller and will help make it clearer what things have changed in these files, if any. :-)

As a skeleton for your to-be-added test, you probably want something along the lines of:
- add some anchor to readerModeArticle.html (make sure it's far enough down that you can tell if the page has scrolled)
- in a new test file:

add_task(function* () {
  yield BrowserTestUtils.withNewTab(<path to readerModeArticle.html#foo>, function* (browser) {
    let browserLoadedPromise = BrowserTestUtils.browserLoaded(browser);
    let readerButton = document.getElementById("reader-mode-button");
    readerButton.click();
    yield browserLoadedPromise;
    Assert.equal(gBrowser.currentURI.ref, "foo", "Ref should be in URL."); // Assuming doing that does end up working.
    yield ContentTask.spawn(browser, null, function* () {
      // check that the anchor element is in the scrolled-into-view area using layout measures (scrollTop, window innerheight, etc.) or something.
    });
  });
});

Comment 13

[Sébastien Blin [:sblin] [:amarok]]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > I'll review properly on Monday, but...
> > 
> > the URL for the page is:
> > 
> > about:reader?url=<escaped-url-of-original-page>
> > 
> > So I would expect %23ref to not actually be reflected in location.hash or
> > the page offset.
> > 
> > Basically what I'm saying is, we should set location.hash (d'oh, not sure
> > why I suggested location.href which is more work) in the same place where
> > you're scrolling to the content right now. I *think* that should work
> > (assuming a matching named/id'd anchor is present, of course).
> 
> ... so then the full URL becomes:
> 
> about:reader?url=<escaped-url-of-original-page-with%23ref>#ref
> 
> if that makes sense. :-)

Yes, location.hash works (but the title/a is not aligned to the top). But in the address bar we will have "#ref#ref" and it's not good. I tried to change with _win.history.pushState but same result and if I rewrite location.href, it will reload the page again and again.


In test/browser_bug1124271_readerModePinnedTab.js, the problem is:
>  yield BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow");

Thx for the tip with "hg move"

Comment 14

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_1.patch

Comment 15

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to :Gijs Kruitbosch from comment #10)
> > > I'll review properly on Monday, but...
> > > 
> > > the URL for the page is:
> > > 
> > > about:reader?url=<escaped-url-of-original-page>
> > > 
> > > So I would expect %23ref to not actually be reflected in location.hash or
> > > the page offset.
> > > 
> > > Basically what I'm saying is, we should set location.hash (d'oh, not sure
> > > why I suggested location.href which is more work) in the same place where
> > > you're scrolling to the content right now. I *think* that should work
> > > (assuming a matching named/id'd anchor is present, of course).
> > 
> > ... so then the full URL becomes:
> > 
> > about:reader?url=<escaped-url-of-original-page-with%23ref>#ref
> > 
> > if that makes sense. :-)
> 
> Yes, location.hash works (but the title/a is not aligned to the top).

Hm, so you're it works (in what way?) but also that assigning to location.hash doesn't actually scroll the page? That's... surprising. :-\

I tried: http://output.jsbin.com/mopukayufa

and executing: location.hash = 'foo'

in the console, and that seemed to work...

> But in
> the address bar we will have "#ref#ref" and it's not good. I tried to change
> with _win.history.pushState but same result and if I rewrite location.href,
> it will reload the page again and again.

Hm, we unescape the %23 ? That's a location bar bug...

We could probably avoid the anchor appearing in the query string... but that only makes sense if assigning to location.hash can be made to actually scroll the page.

> In test/browser_bug1124271_readerModePinnedTab.js, the problem is:
> >  yield BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow");

Hm. Not sure why that would break after moving the file. Can you try changing that code to do:

let pageShownPromise = BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow");
readerButton.click();
yield pageShownPromise;

?

Did you try if the test runs correctly in its original location on your machine?

Comment 16

[Sébastien Blin [:sblin] [:amarok]]

(In reply to :Gijs Kruitbosch from comment #15)
> > 
> > Yes, location.hash works (but the title/a is not aligned to the top).
> 
> Hm, so you're it works (in what way?) but also that assigning to
> location.hash doesn't actually scroll the page? That's... surprising. :-\

My bad, the url was #ref#ref#ref... so it doesn't scroll.

> We could probably avoid the anchor appearing in the query string... but that
> only makes sense if assigning to location.hash can be made to actually
> scroll the page.

I tried something like:

> window.location.hash = "#foo";
> window.history.replaceState("", document.title, window.location.pathname);
This works in a page, but in the reader mode, I can't do :
> this._win.history.replaceState("", this._doc.title, this._win.location.pathname);
because pathname is empty. and all is just in .href...

> Hm. Not sure why that would break after moving the file. Can you try
> changing that code to do:
> 
> let pageShownPromise =
> BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow");
> readerButton.click();
> yield pageShownPromise;
> 
> ?
It works now! Thanks

> Did you try if the test runs correctly in its original location on your
> machine?
Yes and it doesn't work too

I'll try to change this._win.history.replaceState this evening or tomorrow :)

Comment 17

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > We could probably avoid the anchor appearing in the query string... but that
> > only makes sense if assigning to location.hash can be made to actually
> > scroll the page.
> 
> I tried something like:
> 
> > window.location.hash = "#foo";
> > window.history.replaceState("", document.title, window.location.pathname);
> This works in a page, but in the reader mode, I can't do :
> > this._win.history.replaceState("", this._doc.title, this._win.location.pathname);
> because pathname is empty. and all is just in .href...

Yeah, I don't think about: URIs have a .pathname. The best you might be able to do is to use the originalUrl (which we have a getter for, IIRC) and then get the spec of that sans hash).

I don't know if doing that is going to work, anyway - it might cause a page reload. So we might need to just live with it. The alternative would be to fix reader mode to take a URI without the ref in the first place, and to pass the ref separately, but that might be tricky.

Comment 18

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_2.patch

I confirm, the page reloads.

I wrote the test for this bug.

Comment 19

[:Gijs (he/him)]

Comment on attachment 8806433 [details] [diff] [review]
1189491_2.patch

Review of attachment 8806433 [details] [diff] [review]:
-----------------------------------------------------------------

The test looks good (some tiny nits below), but assuming that assigning to location.hash works (apart from making the URL ugly), I would prefer that over the manual looking for elements and calling scrollIntoView() on them. From the earlier comments, I was under the impression that it *did* scroll to the right place. Did I misunderstand?

::: toolkit/components/reader/test/browser_readerMode_with_anchor.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We normally public domain tests, unless you object? If public domain is fine for you, you can omit the license terms entirely. :-)

@@ +7,5 @@
> + * reader-able content.
> + */
> +const TEST_PREFS = [
> +  ["reader.parse-on-load.enabled", true],
> +];

You probably don't need this, but can you add:

"use strict";

?

@@ +18,5 @@
> +    let readerButton = document.getElementById("reader-mode-button");
> +    readerButton.click();
> +    yield pageShownPromise;
> +    yield ContentTask.spawn(browser, null, function* () {
> +      //Check if offset != 0

Nit: space after //

Comment 20

[Sébastien Blin [:sblin] [:amarok]]

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8806433 [details] [diff] [review]
> 1189491_2.patch
> 
> Review of attachment 8806433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test looks good (some tiny nits below), but assuming that assigning to
> location.hash works (apart from making the URL ugly), I would prefer that
> over the manual looking for elements and calling scrollIntoView() on them.
> From the earlier comments, I was under the impression that it *did* scroll
> to the right place. Did I misunderstand?
location.hash works, but I don't like the idea of an ugly URL. I need to find a solution to change the hash without changing the address bar. 

> 
> ::: toolkit/components/reader/test/browser_readerMode_with_anchor.js
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> We normally public domain tests, unless you object? If public domain is fine
> for you, you can omit the license terms entirely. :-)

Other tests are under this license, that's why I copy this comment, but I agree with public domain

> "use strict";
> Nit: space after //
Yeap ok

Comment 21

[:Gijs (he/him)]

Hm, perhaps I've misunderstood the address bar problem. Does the actual about:reader? URL start showing up in the address bar, or something? It's possible we can make the address bar lie about the URL a bit more without affecting anything else...

Comment 22

[Sébastien Blin [:sblin] [:amarok]]

For example, the address bar is gitolite.com/gitolite/gitolite.html#your-server#your-server But I think we can detect the ref before load the article, write gitolite.com/gitolite/gitolite.html (for article.url) and after that change location.hash

Comment 23

[:Gijs (he/him)]

Ah, I see. I think there's a bug in this code:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#139-155

the URLBarSearchParams should only be applied to the URL spec without the hash. Right now it isn't, so we extract the hash twice.

Fixing this properly requires both avoiding the outer hash (that we're adding) initially, and then updating it back into the original URL afterwards, so that we end up with the correct hash in the URL bar if the user clicks anchor links in the document.

You can probably fix the first bit by using something like this:

let outerHash = "";
try {
  let uriObj = Services.io.newURI(url, null, null);
  url = uriObj.specIgnoringRef;
  outerHash = uriObj.ref;
} catch (ex) { /* ignore, use the raw string */ }

which will provide the original URI of the page without the new hash, and stick the hash on the end of the about:reader? URL in the outerHash local variable.

Then a bit later, after we extract the originalUrl, you can create a URI out of that if possible, so instead of encodedURL you can do something like:

let originalUrl = searchParams.get("url");
try {
  originalUrl = decodeURIComponent(originalUrl);
} catch (ex) {
  Cu.reportError(new Error("Error decoding original URL: " + ex));
  return originalUrl;
}
if (outerHash) {
  try {
    let uriObj = Services.io.newURI(originalUrl, null, null);
    uriObj = Services.io.newURI(outerHash, null, uriObj);
    originalUrl = uriObj.spec;
  } catch (ex) {}
}
return originalUrl;


I *think* that should work - but I've not tested it, so there may be some more surprises on the way. :-)

(I would be perfectly fine with fixing the URL stuff in a separate bug if it turns out to be too hairy - we can live with just the duplicated hash for a while if necessary.)

Comment 24

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_3.patch

I totally agree. I found a solution to write the good url address and scroll with this._win.location.hash.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8806718 [details] [diff] [review]
1189491_3.patch

Review of attachment 8806718 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/reader/ReaderMode.jsm
@@ +151,4 @@
>      try {
> +      let uriObj = Services.io.newURI(originalUrl, null, null);
> +      originalUrl = uriObj.specIgnoringRef;
> +      outerHash = uriObj.ref;

Ah, I'm sorry, I think I was unclear.

This method is being passed an `url` parameter, which if it's an about:reader URL will look like:

about:reader?url=<encodedURIwithhash>#hash

I think we should use `Services.io.newURI` and `specIgnoringRef` to remove the hash from that string, *before* using the URLSearchParams to get the originalUrl value. When we do that, we can store the outer hash the same way you're doing here, and then originalUrl will be correct - but also, we won't have to look for the secondHash inside the extracted hash as you do below.

Then we can combine the original URL with the new hash with the "relative" version of the Services.io.newURI invocation (the 2nd param is a charset, always null, the 3rd is the "base URI"):

originalUrl = Services.io.newURI(outerHash, null, originalUrl).spec;

(this can fail, in which case we should just return originalUrl).

Does this make more sense?

Comment 26

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_3.patch

Erf, sorry for the previous patch.

Comment 27

[:Gijs (he/him)]

Comment on attachment 8806912 [details] [diff] [review]
1189491_3.patch

Review of attachment 8806912 [details] [diff] [review]:
-----------------------------------------------------------------

With the changes below, r=me. Thanks so much for sticking to this! I'll push to try and land once you update the patch for the minor issues listed below. :-)

::: toolkit/components/reader/ReaderMode.jsm
@@ +161,1 @@
>      }

Right, although I did realize through your earlier patch (which got rid of the decodeURIComponent call) that URLSearchParams already unescapes URL parameters. So AFAICT we can just ditch this decoding step entirely.

@@ +161,5 @@
>      }
> +    if (outerHash) {
> +      try {
> +        let uriObj = Services.io.newURI(originalUrl, null, null);
> +        uriObj = Services.io.newURI('#' + outerHash, null, uriObj);

We still have to assign uriObj.spec to originalUrl here, I think, or it won't have a hash at all.

Comment 28

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 1189491_3.patch

Woops, don't know why I removed uriObj.spec

Have a nice day,

Comment 29

[:Gijs (he/him)]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5f9d7b6d087a96d52d162eb742b9b5e3899067

Comment 30

[:Gijs (he/him)]

It looks like there are test failures when we run the browser_readerMode.js test. Can you reproduce those locally? I see things like:

 TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Info panel should be anchored at the reader mode button - 

 TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Info panel should have closed - 

 TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Moved one step back in the session history. -

Comment 31

[Sébastien Blin [:sblin] [:amarok]]

I just run a ./mach mochitest toolkit/components/reader/test/browser_readerMode.js and yes, I have these 3 failures...

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da7d22ebcd2
Take anchors into account in the reader view, r=gijs

Comment 33

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #31)
> I just run a ./mach mochitest
> toolkit/components/reader/test/browser_readerMode.js and yes, I have these 3
> failures...

I found a fix for this by not assigning the ref to location.hash if it was empty (which makes sense to me, because it alters the history state) and landed the patch.

Comment 34

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

There were a number of eslint failures < https://treeherder.mozilla.org/logviewer.html#?job_id=38669248&repo=mozilla-inbound >

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9296be2a4f

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb8dac3eb40
Take anchors into account in the reader view, r=gijs

Comment 36

[:Gijs (he/him)]

Relanded with the eslint issues addressed.

Comment 37

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

Sorry, had to backed this out for failing browser_readerMode.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d16d22236f182f41c126b9084512232c5b1ab4de

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1cb8dac3eb40e519ee45a3fc5a9490b889ee5fc7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38723432&repo=mozilla-inbound

05:18:46     INFO - Entering test bound test_reader_button
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Element should not be null, when checking visibility - 
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Reader mode button is not present on a new tab - 
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Info panel shouldn't appear without the reader mode button - 
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Shouldn't have detected the first article - 
05:18:46     INFO - Wait tab event: load
05:18:46     INFO - Buffered messages logged at 05:18:44
05:18:46     INFO - Tab event received: load
05:18:46     INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/toolkit/components/reader/test/readerModeArticle.html" line: 0}]
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Element should not be null, when checking visibility - 
05:18:46     INFO - TEST-PASS | toolkit/components/reader/test/browser_readerMode.js | Reader mode button is present on a reader-able page - 
05:18:46     INFO - Buffered messages finished
05:18:46     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Info panel should be anchored at the reader mode button - 
05:18:46     INFO - Stack trace:
05:18:46     INFO -     chrome://mochitests/content/browser/toolkit/components/reader/test/browser_readerMode.js:test_reader_button:46
05:18:46     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:737:9
05:18:46     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:657:7
05:18:46     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59

Please fix this and resubmit. Thank you.

Comment 38

[:Gijs (he/him)]

Did this not fail last time it landed? :-\

(The failures are different from the ones in the trypush, which I reproduced locally and fixed.)

Comment 39

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #38)
> Did this not fail last time it landed? :-\

Turns out it did, but that wasn't mentioned in the bug so I didn't realize.

> (The failures are different from the ones in the trypush, which I reproduced
> locally and fixed.)

It also turns out that this failure is intermittent on the push to inbound, and doesn't reproduce at all on mac (at least, not on mine, and the inbound push is also green).

I'll see if I can repro on Windows or Linux and fix whatever's breaking. :-\

Comment 40

[:Gijs (he/him)]

I can't reproduce on either Windows or Linux, either. Sigh.

I'll see if I can find time to look into this more deeply next week. Otherwise, Sébastien, if you can reproduce the test failures that would be quite helpeful at this point...

Comment 41

[Sébastien Blin [:sblin] [:amarok]]

Hi!
Sorry for the delay I was not present.

I will read new posts tomorrow and I'll try to reproduce (I'm on linux)

Comment 42

[Sébastien Blin [:sblin] [:amarok]]

I confirm, I've got 2 failures:
The following tests failed:
104 INFO TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Info panel should be anchored at the reader mode button - 
Stack trace:
    chrome://mochitests/content/browser/toolkit/components/reader/test/browser_readerMode.js:test_reader_button:46
    Tester_execTest@chrome://mochikit/content/browser-test.js:733:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:653:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
105 INFO TEST-UNEXPECTED-FAIL | toolkit/components/reader/test/browser_readerMode.js | Info panel should have closed - 
Stack trace:
    chrome://mochitests/content/browser/toolkit/components/reader/test/browser_readerMode.js:test_reader_button:54
    Tester_execTest@chrome://mochikit/content/browser-test.js:733:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:653:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:743:59
SUITE-END | took 49s

Comment 43

[Sébastien Blin [:sblin] [:amarok]]

I think "window" is the cause.

Comment 44

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #43)
> I think "window" is the cause.

Can you clarify what you mean by this? :-)

Comment 45

[Sébastien Blin [:sblin] [:amarok]]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #43)
> I think "window" is the cause.

Forget this, window is correct... UITour.isInfoOnTarget is only used in this test. And UITour is defined in browser/content/. So I think we need to move something else?

Comment 46

[Sébastien Blin [:sblin] [:amarok]]

I move back readerMode.js and it fails... So the test is correct.

Comment 47

[:Gijs (he/him)]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #46)
> I move back readerMode.js and it fails... So the test is correct.

Can you add logs in both the if and else block here:

https://dxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#113-124

and see if they get hit? Do you visually see the reader mode info tour panel (should look something like this: https://bugzilla.mozilla.org/attachment.cgi?id=8738212 - except not so ridiculously tall, of course; screenshot from another bug...) appear while the test runs?

(I would debug it myself but as I can't reproduce... :-\

Comment 48

[Sébastien Blin [:sblin] [:amarok]]

Attachment: 2016-11-07-08:38:48-screenshot.png

Sure,

> Do you visually see the reader mode info tour panel?

Yeap, I see the reader mode info tour panel. He doesn't disappears.

> Can you add logs in both the if and else block here:

For browser_readerMode.js I can see:
In UITour.jsm, tooltip.state = closed in isInfoOnTarget for test line 36,
Then we hit:

if (browser.isArticle &&
        !Services.prefs.getBoolPref("browser.reader.detectedFirstArticle") &&
        currentUriHost && !currentUriHost.endsWith("mozilla.org")) {

Then, in isInfoOnTarget on test line 46, tooltip is also closed.

After   ok(Services.prefs.getBoolPref("browser.reader.detectedFirstArticle"),
     "Should have detected the first article"); in browser_readerMode.js
We hit  } else if (this._readerModeInfoPanelOpen) { in ReaderParent.jsm, and for


  ok(!UITour.isInfoOnTarget(window, "readerMode-urlBar"), "Info panel should have closed"); line 54, tooltip in UITour.jsm is open.

Comment 49

[:Gijs (he/him)]

So it seems we do show the popup, but the test proceeds before it's been shown and as a result it fails. After using a debug build on Linux, I could reproduce locally. Waiting for the popup fixed this for me. Trypush:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47898796d951b879ca61620e9e76c90efc07b524

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87686b5a927c
Take anchors into account in the reader view, r=gijs

Comment 51

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

https://hg.mozilla.org/mozilla-central/rev/87686b5a927c