Browser API: Add canGoBack and canGoForward properties to browser elements

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: benfrancis, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla16
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
When creating a browser as a webapp it is common to want to know whether there is a page to go back to or a page to go forward to in the session history, to decide whether to display back/forward buttons or whether to disable them.

Privacy concerns were made about a previous proposal to allow content to access the index within session history https://bugzilla.mozilla.org/show_bug.cgi?id=708179 so this is an alternative proposal to add canGoBack and canGoForward properties to browser elements/mozbrowser iframes which reveal less information.

This is needed for the Gaia browser to be able to use the platform's session history instead of re-implementing the History API inside the app.
(Reporter)

Updated

6 years ago
Summary: Browser API: Add canGoBack() and canGoForward() properties to browser elements → Browser API: Add canGoBack and canGoForward properties to browser elements
If we do that, for the sack of the platform, can we do that on a different element than iframe?
(Assignee)

Comment 2

6 years ago
This would be an easy one for you to start with, Ben.  Let me know if you want me to step you through it.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 3

5 years ago
It doesn't make much sense to do CanGoBack without GoBack, so I'll probably do both in this bug.
(Assignee)

Updated

5 years ago
Depends on: 764130
Keywords: dev-doc-needed
(Assignee)

Comment 4

5 years ago
Vivien, smaug, et al.: Using a DOMRequest feels pretty awful here:

  iframe.getCanGoBack().onsuccess = function(e) {
    var canGoBack = e.target.result;
    ...
  }

What about if we just use a callback:

  iframe.getCanGoBack(function(canGoBack) {
    ...
  });

For the sake of consistency, I'd probably change our screenshot code too.
(Assignee)

Updated

5 years ago
Depends on: 764232
(Reporter)

Updated

5 years ago
Duplicate of this bug: 708179
(Reporter)

Comment 6

5 years ago
Yeah, canGoBack(callback) was the original suggestion on https://wiki.mozilla.org/WebAPI/BrowserAPI

What's the reason for adding the get prefix? Is this for consistency with something else?
(Assignee)

Comment 7

5 years ago
The |get| suggests to me that we're going through a callback; you can't do |iframe.canGoBack|.  But it's not a big deal either way.

We do need to figure out if we're going to use a DOMRequest, though.
(Assignee)

Updated

5 years ago
Depends on: 764248
(Assignee)

Comment 8

5 years ago
Created attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods
(Assignee)

Comment 9

5 years ago
Created attachment 632849 [details] [diff] [review]
Part 2: Add tests for getCanGo{Back,Forward}
(Assignee)

Comment 10

5 years ago
I left these using a DOMRequest because we don't seem to have quorum to decide whether we want to change it.

Next up: go{Back,Forward}().

Note that canGoBack (and, goBack) only works properly OOP.  It's more work to get it functioning properly in-process, and since we're not planning to run anything in-process, I don't think it's worth bothering with atm.
(In reply to Justin Lebar [:jlebar] from comment #7)
> The |get| suggests to me that we're going through a callback; you can't do
> |iframe.canGoBack|.  But it's not a big deal either way.
> 
> We do need to figure out if we're going to use a DOMRequest, though.

My main issue with DOMRequest in this case is that I don't think you will ever need the DOMRequest.onerror part. So I would suggest to use a callback but it make me sad to not have unified APIs :/
(Assignee)

Comment 12

5 years ago
We never use the onerror part for getScreenshot, either.  But I guess your point is that we might, at some time in the future?  If we made that change in the future, nobody's going to watching onerror anyway...
(Assignee)

Comment 13

5 years ago
Created attachment 632901 [details] [diff] [review]
Part 3: Add goBack/goForward.
(Assignee)

Comment 14

5 years ago
Created attachment 632902 [details] [diff] [review]
Part 4: Tests for goBack / goForward
(Assignee)

Updated

5 years ago
Attachment #632901 - Attachment description: Bug 741755 - Part 3: Add goBack/goForward to <iframe mozbrowser>. → Part 3: Add goBack/goForward.
(Assignee)

Updated

5 years ago
Attachment #632902 - Attachment description: Bug 741755 - Part 4: Tests for goBack / goForward on <iframe mozbrowser>. → Part 4: Tests for goBack / goForward
(Assignee)

Comment 15

5 years ago
This is all straightforward.  The only slightly tricky change is in the first patch; I consolidated the DOMRequest code used by screenshots with the DOMRequest code used here.

smaug, you get this r?, but if you can't get to it by Monday, we can bump it to Mounir.
(Assignee)

Updated

5 years ago
Attachment #632848 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #632849 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #632901 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #632902 - Flags: review?(bugs)
Can't we have synchronous canGoBack and canGoForward? And async goBack()/goForward() returning DOMRequest? Feels like we could easily cache that information to get it quickly, can't we?
(Assignee)

Comment 17

5 years ago
> Can't we have synchronous canGoBack and canGoForward?

shistory doesn't have "can go {back,forward} changed" events, at the moment.

Caching is complicated because things that aren't navigations can change canGoBack/Forward.  For example, removing an iframe can change things.

But I realize that the API we have now is insufficient to write a good back button implementation -- because we're not sending canGoBack events, the embedder has no way to detect when it needs to query canGoBack, except when the top-level page navigates, and that's insufficient.  So maybe we should figure out how to send canGoBack notifications, and then we can also have a synchronous cache, if we want.

> And async goBack()/goForward() returning DOMRequest?

Is the goal to detect failure, in the case when you try to go back but can't?  I'm not sure we care so much...
(In reply to Justin Lebar [:jlebar] from comment #17)
> > Can't we have synchronous canGoBack and canGoForward?
> 
> shistory doesn't have "can go {back,forward} changed" events, at the moment.
> 
> Caching is complicated because things that aren't navigations can change
> canGoBack/Forward.  For example, removing an iframe can change things.
> 
> But I realize that the API we have now is insufficient to write a good back
> button implementation -- because we're not sending canGoBack events, the
> embedder has no way to detect when it needs to query canGoBack, except when
> the top-level page navigates, and that's insufficient.  So maybe we should
> figure out how to send canGoBack notifications, and then we can also have a
> synchronous cache, if we want.

Indeed. Does it really make sense to push .canGoBack and .canGoForward if it is not really fixing the use case we have? Right now it seems like it would be used -at best- like:
f.getCanGoBack().onsuccess = function() {
  f.goBack();
}
Which would be more verbose than:
f.goBack();

> > And async goBack()/goForward() returning DOMRequest?
> 
> Is the goal to detect failure, in the case when you try to go back but
> can't?  I'm not sure we care so much...

The goal is to know when the action is done. Given that you can also call that method without being allowed to go back/forward, being tell when that happens wouldn't hurt.
(Assignee)

Comment 19

5 years ago
> Right now it seems like it would be used -at best- like:
> f.getCanGoBack().onsuccess = function() {
>   f.goBack();
> }
> Which would be more verbose than:
> f.goBack();

Well, no.  If you wanted to go back, you'd just goBack(), and if it fails, then whatever.

> Given that you can also call that method without being allowed to go back/forward, being tell when 
> that happens wouldn't hurt.

I guess.  Note that we silently ignore history.back() calls when you can't go back.
(In reply to Justin Lebar [:jlebar] from comment #19)
> > Right now it seems like it would be used -at best- like:
> > f.getCanGoBack().onsuccess = function() {
> >   f.goBack();
> > }
> > Which would be more verbose than:
> > f.goBack();
> 
> Well, no.  If you wanted to go back, you'd just goBack(), and if it fails,
> then whatever.

That was actually my point (very badly formed): there is no reason to use getCanGoBack() because the developer doesn't get informed about a state change so the only reason to use it would be before going back which is actually useless.
Or do we expect developers to call the method regularly in a setInterval? Or after each location change?
(Assignee)

Comment 21

5 years ago
> there is no reason to use getCanGoBack() [...]
> Or do we expect developers to call the method regularly in a setInterval? Or after each location 
> change?

The purpose of getCanGoBack is mostly so that we can enable/disable the back button as appropriate.  But like I said earlier, we really need an event that says "back button will work now," because we don't want to use an interval, and the back button can become disabled after something that's not a locationchange (e.g. iframe removal).
(Assignee)

Comment 22

5 years ago
Comment on attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods

Clearing review request; per discussion with Mounir, I think this needs to be reworked.
Attachment #632848 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #632849 - Flags: review?(bugs)
(Assignee)

Comment 23

5 years ago
Comment on attachment 632901 [details] [diff] [review]
Part 3: Add goBack/goForward.

We can still have goBack without canGoBack, though, so I'm leaving this r? here.  It would be nice to use real shistory in the B2G browser sooner rather than later.

Updated

5 years ago
Attachment #632901 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #632902 - Flags: review?(bugs) → review+
(Assignee)

Comment 24

5 years ago
It looks like desktop Firefox queries CanGoBack/CanGoForward whenever it gets an onlocationchange event.  It's not clear to me whether we're talking onlocationchange in the top-level frame or onlocationchange in any frame.

But I think given that, we can add canGoBack to mozbrowser and have the Gaia browser query whenever it gets an onlocationchange event (which is only for the top-level browsing context).  It's not 100% correct, and we can fix it later, but it's more important that Gaia have something 95% correct, right now.
(Assignee)

Updated

5 years ago
Attachment #632848 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #632849 - Flags: review?(mounir)
Comment on attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods

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

::: dom/browser-element/BrowserElementChild.js
@@ +337,5 @@
>      }
>    },
>  
> +  _recvCanGoBack: function(data) {
> +    var webNav = docShell.QueryInterface(Ci.nsIWebNavigation);

Shouldn't you check that webNav isn't null?

@@ +345,5 @@
> +    });
> +  },
> +
> +  _recvCanGoForward: function(data) {
> +    var webNav = docShell.QueryInterface(Ci.nsIWebNavigation);

ditto
Attachment #632848 - Flags: review?(mounir) → review+
(Assignee)

Comment 26

5 years ago
> Shouldn't you check that webNav isn't null?

When QI fails in JS, don't you get an exception?  But don't all docshells implement nsIWebNavigation?
Comment on attachment 632849 [details] [diff] [review]
Part 2: Add tests for getCanGo{Back,Forward}

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

::: dom/browser-element/mochitest/browserElement_CanGoBack.js
@@ +15,5 @@
> +  // crosses the mozbrowser boundary.  It's like the mozbrowser wasn't there;
> +  // canGoBack reflects whether the top-level frame can go back, not whether the
> +  // iframe itself can go back.
> +  if (!browserElementTestHelpers.getOOPByDefaultPref()) {
> +    ok(true, "Nothing to do here.");

I would put:
ok(false, "We should not be there!");
Given that there is only an OOP test file.

@@ +37,5 @@
> +
> +function checkCanGoBackAndForward(canGoBack, canGoForward, nextTest) {
> +  var seenCanGoBackResult = false;
> +  iframe.getCanGoBack().onsuccess = function(e) {
> +    seenCanGoBackResult = true;

Can you add is(seenCanGoBackResult, false, "The success handler shouldn't be called twice.");
before seenCanGoBackResult = true;

@@ +44,5 @@
> +  };
> +
> +  var seenCanGoForwardResult = false;
> +  iframe.getCanGoForward().onsuccess = function(e) {
> +    seenCanGoForwardResult = true;

same here
Attachment #632849 - Flags: review?(mounir) → review+
Comment on attachment 632902 [details] [diff] [review]
Part 4: Tests for goBack / goForward

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

::: dom/browser-element/mochitest/browserElement_CanGoBack.js
@@ +89,5 @@
> +  addOneShotIframeEventListener('mozbrowserlocationchange', function(e) {
> +    is(e.detail, browserElementTestHelpers.emptyPage2);
> +    checkCanGoBackAndForward(true, false, SimpleTest.finish);
> +  });
> +  iframe.goForward();

Can you add a test that test the situation where you can go back *and* forward?
(In reply to Justin Lebar [:jlebar] from comment #26)
> > Shouldn't you check that webNav isn't null?
> 
> When QI fails in JS, don't you get an exception?

Maybe. I very rarely use JS in Firefox chrome.

> But don't all docshells implement nsIWebNavigation?

Sure. We can actually assume that if that changes, it will be caught by your tests.

Forget that comment.
(Assignee)

Comment 30

5 years ago
> Can you add a test that test the situation where you can go back *and* forward?

I will, but I'll add it to the second set of tests (part 4) -- it's easier to get into this situation when you have goBack().
(In reply to Justin Lebar [:jlebar] from comment #30)
> > Can you add a test that test the situation where you can go back *and* forward?
> 
> I will, but I'll add it to the second set of tests (part 4) -- it's easier
> to get into this situation when you have goBack().

The comment was made on part 4 actually ;)
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ce1a042083
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0f3399cea3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21ba17e03af
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c540b02399
https://hg.mozilla.org/mozilla-central/rev/c9ce1a042083
https://hg.mozilla.org/mozilla-central/rev/2a0f3399cea3
https://hg.mozilla.org/mozilla-central/rev/b21ba17e03af
https://hg.mozilla.org/mozilla-central/rev/31c540b02399
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getCanGoBack
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getCanGoForward
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.goBack
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.goForward
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.