Bugzilla

Comment 5

•

7 years ago

Mike, any idea what would cause this? FWIW, reader mode depends on the pageshow and DOMContentLoaded event - see https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tab-content.js#318 .

Blocks: 1261842

status-firefox53: fix-optional → wontfix

status-firefox54: fix-optional → wontfix

status-firefox56: --- → fix-optional

Flags: needinfo?(mconley)

Priority: -- → P2

Comment 6

•

7 years ago

Hm, it seems that the MozAfterPaint event is not firing in this scenario, until you hover a link (since the underline is being painted under a link on this page):

http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/base/content/tab-content.js#379

I don't know why bug 1261842 would affect this, but I think I understand why we're not firing MozAfterPaint - we're just moving the layers from one tab to another, which isn't requiring a full-blown repaint.

Could we kick off this function: http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/base/content/tab-content.js#382

on pageshow if persisted?

Flags: needinfo?(mconley) → needinfo?(gijskruitbosch+bugs)

Comment 7

•

7 years ago

(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #6)
> Hm, it seems that the MozAfterPaint event is not firing in this scenario,
> until you hover a link (since the underline is being painted under a link on
> this page):
> 
> http://searchfox.org/mozilla-central/rev/
> 5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/base/content/tab-content.
> js#379
> 
> I don't know why bug 1261842 would affect this, but I think I understand why
> we're not firing MozAfterPaint - we're just moving the layers from one tab
> to another, which isn't requiring a full-blown repaint.
> 
> Could we kick off this function:
> http://searchfox.org/mozilla-central/rev/
> 5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/base/content/tab-content.
> js#382
> 
> on pageshow if persisted?

Is pageshow guaranteed to fire after a layout has happened? We already have an extant persisted-pageshow->wait-for-paint->run path to that function. It's not clear to me that in 'normal' (ie non-tab-moving) scenarios, running it directly after pageshow isn't going to run into issues we've had before with this code (where the frames of various nodes are empty because no layout has happened at all - this is why we're waiting for paint).

Could we just copy over the reader mode flag when swapping docshells/browsers together with other state we copy? That seems less risky.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)

Comment 8

•

7 years ago

(In reply to :Gijs from comment #7)
> Could we just copy over the reader mode flag when swapping
> docshells/browsers together with other state we copy? That seems less risky.

Oh yeah, that's way better. Let's do that! :) good-first-bug?

Flags: needinfo?(mconley)

Comment 9

•

7 years ago

To fix this bug, it should be enough to copy the 'isArticle' property from one browser to another in this block of code:

https://dxr.mozilla.org/mozilla-central/rev/b07db5d650b7056c78ba0dbc409d060ec4e922cd/browser/base/content/tabbrowser.xml#3261-3272

iff isArticle is true - and, potentially, to call ReaderParent.updateReaderButton() with the new browser. This would need some careful testing. It may be possible to create an automated test for it based on the tests that already exist in https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/test/ .

Mentor: gijskruitbosch+bugs

Keywords: good-first-bug

Summary: [e10s] Reader button is missing after moving tab to new window → [e10s] Reader button is missing after moving tab to new window (see comment 9)

Whiteboard: e10s → [instructions for fixing in comment #9]

Graham Perrin

Comment 10

•

6 years ago

> Reader button is missing after moving tab to new window 

Not reproducible with Firefox 59.0.2_4,1 on FreeBSD-CURRENT with a content process limit of 4. 

Tested with <https://medium.com/firefox-test-pilot/min-vid-graduation-report-9ad74dc37c1> in a two-tab window with about:preferences adjacent. In the new window, the button appears within a split-second.  

Please, was this bug fixed?

Comment 11

•

6 years ago

I can still reproduce the bug as filed with today's nightly (on macOS), with the same issues as described in comment 6 and later.

Status: NEW → RESOLVED

Closed: 6 years ago

Resolution: --- → FIXED

Updated

•

6 years ago

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Comment 12

•

5 years ago

So if it is not solved, can I try to solve it?

Comment 13

•

5 years ago

(In reply to Helenatxu from comment #12)

So if it is not solved, can I try to solve it?

Sure. There's suggestions on what to do in comment 9. Let me know if you need more help.

Comment 14

•

5 years ago

Ok,thank you! I will try and if I have questions I will let you know.

Comment 15

•

5 years ago

Hello! I'm an Outreachy applicant, and I've started working on this problem recently. It appears that the relevant code has been moved to /browser/base/content/tabbrowser.js, as opposed to the xml file. I've managed to get the Reader button to appear for a slight moment before disappearing again, and repeating the same actual behavior (it doesn't re-appear until a link is hovered).

The code I've added in particular is in /browser/base/content/tabbrowser.js, right below the snippet at line 3293.

    // Fix for bug 1350197
    // Copy the isArticle property, and if it's true, 
    // update the reader button for immediate display.
    if (otherBrowser.isArticle) {
      ourBrowser.isArticle = otherBrowser.isArticle;
      ReaderParent.updateReaderButton(ourBrowser);
      // Trying a force doesn't seem to work either.
      ReaderParent.forceShowReaderIcon(ourBrowser);
    }

Would appreciate any further guidance on the problem!

Comment 16

•

5 years ago

(In reply to Roxanne Printice from comment #15)

Hello! I'm an Outreachy applicant, and I've started working on this problem recently. It appears that the relevant code has been moved to /browser/base/content/tabbrowser.js, as opposed to the xml file. I've managed to get the Reader button to appear for a slight moment before disappearing again, and repeating the same actual behavior (it doesn't re-appear until a link is hovered).

The code I've added in particular is in /browser/base/content/tabbrowser.js, right below the snippet at line 3293.
    // Fix for bug 1350197
    // Copy the isArticle property, and if it's true, 
    // update the reader button for immediate display.
    if (otherBrowser.isArticle) {
      ourBrowser.isArticle = otherBrowser.isArticle;
      ReaderParent.updateReaderButton(ourBrowser);
      // Trying a force doesn't seem to work either.
      ReaderParent.forceShowReaderIcon(ourBrowser);
    }
Would appreciate any further guidance on the problem!

Hello Roxanne, I'm sorry but I'm also an Outreachy applicant and I asked before you to work on this bug. I don't think it's right to work on a bug without asking first.

Comment hidden (off-topic)

Comment 18

•

5 years ago

(In reply to Helena Moreno (aka helenatxu) from comment #16)

Hello Roxanne, I'm sorry but I'm also an Outreachy applicant and I asked before you to work on this bug. I don't think it's right to work on a bug without asking first.

My apologies. Do you you mind sharing what you've managed to figure out so far on this bug? I've hit a wall with my solution personally and am curious how others would approach it.

Comment hidden (off-topic)

Comment 20

•

5 years ago

(In reply to Roxanne Printice from comment #18)

(In reply to Helena Moreno (aka helenatxu) from comment #16)

Hello Roxanne, I'm sorry but I'm also an Outreachy applicant and I asked before you to work on this bug. I don't think it's right to work on a bug without asking first.

My apologies. Do you you mind sharing what you've managed to figure out so far on this bug? I've hit a wall with my solution personally and am curious how others would approach it.

Don't worry, I've been assigned to another bug, so you can have this one if you have already started with it. But remember first to ask before working in anything.

Comment 21

•

5 years ago

•

Edited

(In reply to Roxanne Printice from comment #15)

Hello! I'm an Outreachy applicant, and I've started working on this problem recently. It appears that the relevant code has been moved to /browser/base/content/tabbrowser.js, as opposed to the xml file.

Yes, the code has moved, sorry for not updating the comment here.

I've managed to get the Reader button to appear for a slight moment before disappearing again, and repeating the same actual behavior (it doesn't re-appear until a link is hovered).

OK. So something else is calling updateReaderButton a second time and setting things back to false. You could add Cu.reportError("Called updateReaderButton") in the updateReaderButton function, and that'll log to the browser console (cmd-shift-j on mac, ctrl-shift-j on windows/linux) and give you a stacktrace so we can figure out what is doing that, and what to do about it.

Assignee: nobody → stellarluminant

Comment hidden (off-topic)

Comment 23

•

5 years ago

(In reply to :Gijs (he/him) from comment #21)

OK. So something else is calling updateReaderButton a second time and setting things back to false. You could add Cu.reportError("Called updateReaderButton") in the updateReaderButton function, and that'll log to the browser console (cmd-shift-j on mac, ctrl-shift-j on windows/linux) and give you a stacktrace so we can figure out what is doing that, and what to do about it.

Alright... so I've managed to gather this much from logging updateReaderButton calls. My sequence of actions was:

Run Firefox from mozilla-central build using ./mach run --jsconsole
Open new tab
Type https://en.wikipedia.org/wiki/Cereal into address bar
Allow page to fully load. (Reader Button appears, as expected)
Right click wikipedia tab, click Move Tab > Move to New Window (Reader Button is absent)
Hover a link in new window. (Reader Button appears, as expeced)

updateReaderButton is being called in the following functions:

swapBrowsersAndCloseOther in browser/base/content/tabbrowser.js:3300 (this is expected as it's the call I'm making)

onLocationChange in browser/base/content/browser.js:4864
This is being called multiple times, by different functions:
swapBrowsersAndCloseOther in browser/base/content/tabbrowser.js:3324
swapBrowsersAndCloseOther in browser/base/content/tabbrowser.js:3429

receiveMessage in browser/modules/ReaderParent.jsm:47
Once after the browser swap happens, this is called. It's called again after I hover over a link. I don't know how to deduce the source of messages.

Comment 24

•

5 years ago

Thanks for investigating further! Needinfo'ing myself to get back to you (feel free to flag me for needinfo yourself in future) - it's midnight here atm so I'll pick this up tomorrow).

Flags: needinfo?(gijskruitbosch+bugs)

Comment 25

•

5 years ago

(In reply to Roxanne Printice from comment #23)

updateReaderButton is being called in the following functions:

<snip>
receiveMessage in browser/modules/ReaderParent.jsm:47
Once after the browser swap happens, this is called. It's called again after I hover over a link. I don't know how to deduce the source of messages.

OK. So these are "IPC" (inter-process communication) messages. The web content runs in a separate process, and it's sending messages to the "parent" process, which is in charge of the browser UI. There's more information at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Multiprocess_Firefox/Message_Manager/Communicating_with_frame_scripts .

In this case, the message is "Reader:UpdateReaderButton", which if you plug it into searchfox turns up senders (callers of sendAsyncMessage) in mobile/android (which is Firefox on Android, so we can ignore that here) and browser/actors/AboutReaderChild.jsm , https://searchfox.org/mozilla-central/source/browser/actors/AboutReaderChild.jsm .

There are four cases where we send an update there:

2 of them are in response to a "pageshow" or "DOMContentLoaded" event, after we've repainted the window (so we're waiting for a "MozAfterPaint" event as well).
1 of them is when the actual reader mode document loads, in response to the "AboutReaderContentLoaded" event, a custom event sent by reader mode code.
1 of them is in the "pagehide" event.

What's happening here, I think, is that when we move the tab to a new window, we're firing a pagehide event and then a pageshow event, for the same document. But in the pageshow case, we'll also wait for a MozAfterPaint event, so the reader mode button isn't updating until there's another paint, but as was discussed in comment #6, no paint happens until you hover over links or do something else that forces us to repaint the page.

So there's a few things we could try here:

somehow figure out that the pagehide is for a tab detaching. I don't know that this is possible: I don't think the child process JS we're in knows anything about the fact that the tab is detaching.
persist the state on the parent somehow. This is hard/impossible because we can't predict the timing of the pagehide event happening in the child, making it up to the parent via the IPC message (IPC messages are always in-order, but the timing in respect to things that stay in the same (parent) process, like the tab detaching, is not guaranteed).
try to detect the case in the child when the pageshow event fires.

I think the last option is the simplest (for some meaning of simple...).

What we can do is, on the AboutReaderChild class, we can add a _lastPageState property that we initialize to null in the constructor.

In the onPaintWhenWaitedFor(forceNonArticle, event) method, we can set this property to an object with two properties (right before the if statement in whose blocks we're calling this.mm.sendAsyncMessage:

a weak reference to the document, maybe call the prop weakDoc. You can get a weak reference using Cu.getWeakReference(this.content.document). We need it to be a weak reference so that we don't keep hold of the document forever (which would leak memory). The weak reference will just start returning null when the document is released, instead of forcing the document to stay alive.
whether it was readerable, ie the return value of the Readerable.isProbablyReaderable(this.content.document) check.

Then in the pageshow handler, in the aEvent.persisted case, instead of always calling updateReaderButton, we can do something like this:

if (aEvent.persisted) {
  let oldDoc = this._lastPageState && this._lastPageState.weakDoc;
  oldDoc = oldDoc && oldDoc.get();
  if (oldDoc && oldDoc == this.content.document && this._lastPageState.isArticle) {
    // we know the state already from the last time we checked. Just update with that state:
    this.mm.sendAsyncMessage("Reader:UpdateReaderButton", {isArticle: this._lastPageState.isArticle});
    return;
  }
  // We don't know the state, check after the next paint:
  this.updateReaderButton();
}

That should hopefully deal with this...

Note that this is clearly not really a good-first-bug anymore, because this is significantly more tricky than I thought it was going to be... Let me know if you have any questions or if anything is unclear!

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(stellarluminant)

Keywords: good-first-bug

Comment 26

•

5 years ago

I think that did it! I've done quick tests for both positive and negative cases (swapping browsers for both articles and non-articles) and it works as intended.

The sum of the changes with debug logging removed:

below the snippet at /browser/base/content/tabbrowser.js:3293, method swapBrowsersAndCloseOther.

    // Fix for bug 1350197
    // Copy the isArticle property, and if it's true, 
    // update the reader button for immediate display.
    if (otherBrowser.isArticle) {
      ourBrowser.isArticle = otherBrowser.isArticle;
      ReaderParent.updateReaderButton(ourBrowser);
    }

Out of curiosity, I tried removing the snippet above after seeing that the newest solution worked, but that caused the new window to open without reader mode and quickly "flicker" back to reader mode. It seems that this is also a required bit.

at /browser/actors/AboutReaderChild.jsm, method onPaintWhenWaitedFor

    const readerable = Readerable.isProbablyReaderable(this.content.document);

    // Fix for bug 1350197
    // _lastPageState allows us to be able to reference a past state of the page
    // in case of changes like swapBrowsersAndCloseOther.
    this._lastPageState = {
      weakDoc: Cu.getWeakReference(this.content.document),
      isArticle: readerable
    };

    // Only send updates when there are articles; there's no point updating with
    // |false| all the time.
    if (readerable) {
    ...

at /browser/actors/AboutReaderChild.jsm, method handleEvent, switch case pageShow

        if (aEvent.persisted) {
          // Fix for bug 1350197
          // We'll be able to check if the old document was an 
          // article before sending out updateReaderButton .
          let oldDoc = this._lastPageState && this._lastPageState.weakDoc;
          oldDoc = oldDoc && oldDoc.get();
          if (oldDoc && oldDoc == this.content.document && this._lastPageState.isArticle) {
            // we know the state already from the last time we checked. Just update with that state:
            this.mm.sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._lastPageState.isArticle });
            return;
          }
          // We don't know the state, check after the next paint:
          this.updateReaderButton();
        }

I'm unsure of how to approach the automated tests for this, or how else to move forward now.

Flags: needinfo?(stellarluminant) → needinfo?(gijskruitbosch+bugs)

Comment 27

•

5 years ago

One more change that I forgot to include in last message's log:

the constructor for /browser/actors/AboutReaderChild.jsm

  constructor(dispatcher) {
    super(dispatcher);

    this._articlePromise = null;
    this._isLeavingReaderableReaderMode = false;
	this._lastPageState = null;
  }

BugBot [:suhaib / :marco/ :calixte]

Comment 28

•

5 years ago

Looks OK at a glance, but can you please submit a patch through phabricator? :-)

Note: some of your changes look like they have tabs or otherwise irregular indenting in them; you'll want to fix that before submitting the patch.

For the test, let's leave that for a follow-up bug.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(stellarluminant)

Comment 29

•

2 years ago

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: stellarluminant → nobody

Flags: needinfo?(gijskruitbosch+bugs)

Comment 30

•

2 years ago

irwp has expressed an interest in driving this over the line.

Assignee: nobody → irwp

Flags: needinfo?(stellarluminant)

Flags: needinfo?(gijskruitbosch+bugs)

irwp

Assignee

Comment 31

•

2 years ago

Hi there! I was not able to reproduce this bug in the nightly build that I was working with on Ubuntu. When I click Move Tab > Move to New Window, the reader view button is there (and no flickering or anything). Also, the reader view state is maintained when I move a tab to a new window -- so if I'm looking at a tab in reader mode and then move that tab to a new window, it will be in reader mode in that new window (nice!).

From what I can tell there are no spots in the code that implement the suggestions made by Roxanne Printice or by :Gijs, but it might be that I just am not seeing them. The code in tabbrowser.js appears to have changed somewhat in the past 3 years -- e.g., the isArticle property is not used in tabbrowser.js anymore.

I am not familiar enough with the code to know exactly where the right calls are being made so that the reader view button always shows and doesn't flicker. However, in https://searchfox.org/mozilla-central/source/browser/actors/AboutReaderChild.jsm#160 there is a comment and some code that might be relevant:

   * NB: this function will update the state of the reader button asynchronously
   * after the next mozAfterPaint call (assuming reader mode is enabled and
   * this is a suitable document). Calling it on things which won't be
   * painted is not going to work.
   */
  updateReaderButton(forceNonArticle) {
    if (!this.canDoReadabilityCheck()) {
      return;
    }

    this.scheduleReadabilityCheckPostPaint(forceNonArticle);
  }

I'm happy to keep digging or testing further if necessary, or whether the status of the bug should be changed.

Flags: needinfo?(mconley)

Flags: needinfo?(gijskruitbosch+bugs)

Comment 32

•

2 years ago

I concur, I haven't been able to reproduce this bug either. It's possible this has been fixed in the meantime via other means. I'm going to close this one out as WORKSFORME for now.

Thanks for investigating, irwp!

Status: REOPENED → RESOLVED

Closed: 6 years ago → 2 years ago

Flags: needinfo?(mconley)

Flags: needinfo?(gijskruitbosch+bugs)

Resolution: --- → WORKSFORME