815715 - Back button state not updated for history.pushState

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We only update the back/forward button states when we get start and stops on the current document:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#367

but we don't get them for pushState messages. We probably need to send out some message from HandleSessionHistoryMessage("New"):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#463

Comment 1

[Wesley Johnston (:wesj)]

I see this on about:addons, I think because we don't update the url in this case either (i.e. we don't get location change messages either?).

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I'm not sure I love this, but I think its the best we can do. If there's a history state change to a url that's the same as the current one, it basically sends a "fake" location change event.

Comment 3

[Wesley Johnston (:wesj)]

mfinkle ping? I'm going to unbitrot this and push it to you for review.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 685734 [details] [diff] [review]
Patch v1

I don't think desktop Firefox needs to do this? Why does their back button work?

Comment 5

[Wesley Johnston (:wesj)]

They force an update of the back/forward button during locationChange events:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4407

we should be able to do something similar here.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Add some code:

updateBackButton(tab.canDoBack());
updateForwardButton(tab.canDoForward());

right here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#424

or maybe on line 426

?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

We need to notify listeners of this location change even if we're still looking at the same document.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 737731 [details] [diff] [review]
Patch

Some potential regressions:

Will this break favicons that are currently loading for the page?
http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/BrowserApp.java#l151

Won't this cause the site security icon to be animated every time an anchor is clicked or the page pushes a history state?
http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/BrowserToolbar.java#l468

Do we want to remove doorhangers since we're still on the same page?
http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/DoorHangerPopup.java#l126

Comment 10

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Will this break favicons that are currently loading for the page?
> http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/
> BrowserApp.java#l151

Maybe. You'd have to hit a link between the time the page load finished and before the link was opened. I'm not sure about why we do this cancelling anyway. This is all done on a different thread, but might affect page load? Otherwise, I think we still want the favicon?
 
> Won't this cause the site security icon to be animated every time an anchor
> is clicked or the page pushes a history state?
> http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/
> BrowserToolbar.java#l468

No. 1.) We only animate if we're changing state. 2.) If it did change state I think users would expect it to animate.
 
> Do we want to remove doorhangers since we're still on the same page?
> http://hg.mozilla.org/mozilla-central/file/41bb93c802d1/mobile/android/base/
> DoorHangerPopup.java#l126

Yes. Pages use this feature (push and popState) to simulate history. I think they'd expect everything to act exactly as if you had gone to a new page. For # links, its a little stranger, but I think users probably also don't understand a significant difference between them and normal links.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 737731 [details] [diff] [review]
Patch

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

(In reply to Wesley Johnston (:wesj) from comment #10)
>
> Yes. Pages use this feature (push and popState) to simulate history. I think
> they'd expect everything to act exactly as if you had gone to a new page.
> For # links, its a little stranger, but I think users probably also don't
> understand a significant difference between them and normal links.

I don't think links are a concern since tapping them will make doorhangers disappear anyway. For both doorhangers and favicons, I was more concerned about pages that automatically redirect to a hash URL or do a pushState() when loaded without any user interaction. I'm not sure how common this is, though, so I guess we can reevaluate this later if we notice any problems.

Comment 12

[Wesley Johnston (:wesj)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9f4eb0e394e