Closed Bug 1218456 Opened 9 years ago Closed 5 years ago

HTMLAnchorElement.click() does not trigger if element is not in document DOM

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Platform:
All
Other
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox-esr68 --- fixed
firefox70 --- fixed

People

(Reporter: AdamK117, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [specification][type:bug])

Attachments

(6 files)

Bug 1218456 - Allow navigating when there's no pres context. r=smaug
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-esr68+
Details | Review
Bug 1218456 - Document::GetContainer shouldn't be virtual. r=smaug
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-esr68+
Details | Review
Bug 1218456 - Remove nsILinkHandler. r=smaug
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-esr68+
Details | Review
Bug 1218456 - Simplify CheckHandleEventForAnchorsPreconditions. r=smaug
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-esr68+
Details | Review
Bug 1218456 - Remove now-unneeded workarounds in cross-origin navigation tests. r=smaug
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-esr68+
Details | Review
Bug 1218456 - Prevent navigating away in test_overlay.html. r=zbraniecki
47 bytes, text/x-phabricator-request
Details | Review 
What did you do?
================
1) var a = document.createElement("a");
2) a.href = "http://google.com";
3) a.click(); // Does nothing
3) document.body.appendChild(a);
4) a.click(); // Works

What happened?
==============
Only works if the HTMLAnchorElement is appended into the document DOM.

What should have happened?
==========================
Unsure of DOM spec, but my intuition is that HTMLAnchorElement should navigate to href when calling .click().

Is there anything else we should know?
======================================
Firefox 41.0.2

This issue can make constructing Blob URLs using HTMLAnchorElements (and their download attribute) annoying. 

Instead of being able to write:

a.download = "myfile.csv";
a.click();

What's required for firefox is:

a.download = "myfile.csv";
a.document.body.appendChild(a);
a.click();
a.document.body.removeChild(a);

HTMLAnchorElement.click() works for non-DOM elements in Google Chrome 46.0.2490.80 (64b), if that's anything to go by.
Product: Mozilla Developer Network → Developer Documentation
This is not a documentation bug, but a Gecko bug report.
Component: General → DOM: Core & HTML
Product: Developer Documentation → Core
What does Edge do?

I'm very surprised about Chrome's behavior. But that seems to be what the spec says, at least in some cases.
Though
var a = document.implementation.createHTMLDocument().createElement("a"); a.href ="http://www.mozilla.org"; a.click();
seems to segfault in the spec (no browsing context), or am I missing something.
Flags: needinfo?(annevk)
> or am I missing something.

The definition of "fully active" should indeed handle the case of a document with no browsing context, and doesn't..

For the rest of this, I could have sworn I'd seen a similar bug report in the past, but I'm not finding it.
Filed https://github.com/whatwg/html/issues/388 to get that cleared up.
Flags: needinfo?(annevk)
(Should be clear now.)
OK.  Spec just (8 hours ago) got clarified in https://github.com/whatwg/html/issues/2615 to say that this should work on `<a>` only, but not other potentially-navigating elements.  There are various web platform tests:

http://w3c-test.org/html/semantics/links/following-hyperlinks/active-document.window.html
http://w3c-test.org/html/semantics/links/following-hyperlinks/activation-behavior.window.html
http://w3c-test.org/html/semantics/forms/form-submission-0/submission-checks.window.html

Looks like we already had bug 1037610 tracking this, but this one has more information, so I'll dup forward.
Status: UNCONFIRMED → NEW
Ever confirmed: true
So most immediately, nsGenericHTMLElement::Click passes a null prescontext to event handling because it uses GetComposedDoc().  And then nsGenericHTMLElement::CheckHandleEventForAnchorsPreconditions bails out if no prescontext.

It would be pretty simple to change nsGenericHTMLElement::Click to get a prescontext from the owner doc, but this would not match the spec for click() on an <a> in a display:none iframe (not tested upstream, looks like), and would possibly change the behavior of click() on other HTML elements.

So my temptation is to remove that prescontext check from CheckHandleEventForAnchorsPreconditions (we have XXX comments about doing that already) and make the bits that follow work even without a prescontext.
Priority: -- → P3

This bit me when fixing bug 1440537.

Flags: needinfo?(emilio)
See Also: → 1440537
See Also: → 1547524

So I thought this would be mostly straight-forward, and I had a patch that almost fixes it.

Mostly this, but beware of the TODOs which could make it more tricky:

https://hg.mozilla.org/try/rev/ae71a0cdc09ff349e5fa2eba7ed76408cc586d13

The only think that's blocking me from fixing this is basically that we do the navigation off the DOMActivate event (rather than click), and the fact that we handle that event via the pres shell stuff:

https://searchfox.org/mozilla-central/rev/444ee13e14fe30451651c0f62b3979c76766ada4/dom/base/Element.cpp#3136

Other browsers don't seem to dispatch DOMActivate when clicking on links. I see code dispatching those events for more nodes, but for links they don't go through HTMLElement::DefaultEventHandler:

Which is the only thing it could make it end there:

Also I'd note we barely have tests for that event.

So some paths forward in order of less to most difficulty / uncertainity (IMO):

  • Stop dispatching DOMActivate for link clicks and just do the navigation on click.
  • Maybe use plain EventDispatcher::Dispatch to dispatch DOMActivate. It's not clear to me why would it need the pres shell stuff (except we use to check stuff, but I think we should just use Event.target and co. instead of ESM / CurrentEventInfo.
  • Hoist HandleDOMEventWithTarget and to Document (not sure that's a great idea).

Masayuki, do you have any advice here? I'm happy to do any of the above (or none if you have better ideas!). Thanks in advance!

Flags: needinfo?(emilio) → needinfo?(masayuki)

Wow, really sorry for the long delay to reply. This was dropped from my queue accidentally.

According to the UI Events, we should keep firing DOMActivate event on anchor elements. I think that keeping it must be safer from point of view of backward compatibility. And if the link should cause navigation, the click event case of Element::PostHandleEventForLinks() should fall back to activate event case.

I don't think that it's a good way to use EventDispatcher::Dispatch() directly because looks like that it and handlers do not assume given pres context may be null.

And I think that smaug also should check the idea.

Flags: needinfo?(masayuki) → needinfo?(bugs)

It seems there may be versions of a library named imagem still in use which do not properly handle download buttons for generated images, as seen on http://invert.imageonline.co/

Webcompat Priority: --- → ?
See Also: → https://webcompat.com/issues/34234

Using plain EventDispatcher::Dispatch for DOMActivate should be fine, but need to then stop relying on prescontext in various places.

Flags: needinfo?(bugs)

Ok, I'll try to get time to work on this next week then. The prescontext dependency is mostly artificial, docshell is mostly what we need.

Assignee: nobody → emilio
Flags: needinfo?(emilio)

It's never overridden. Also chances are we should remove it and just use
GetDocShell().

Depends on D37404

Interfaces with just one implementation don't seem very useful.

Depends on D37405

I've checked that this doesn't change behavior in the test-case for bug 30178,
which is just an imagemap inside a link.

Overall it'd be weird if we needed this for area-inside-a but not for stuff like
nested links.

Depends on D37406

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/343046089f8e
Allow navigating when there's no pres context. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e3da86278ecf
Document::GetContainer shouldn't be virtual. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e5d37afff36a
Remove nsILinkHandler. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8bd57ebc4528
Simplify CheckHandleEventForAnchorsPreconditions. r=smaug
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60e865d21df5
Remove now-unneeded workarounds in cross-origin navigation tests. r=smaug
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/31afe89c2d42
followup: Fix MinGW build bustage.
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f0c24b50dfc
Backed out 5 changesets for Crashtest failures on dom/l10n/tests/mochitest/dom_localization/test_overlay.html. CLOSED TREE

I'm not sure why the test structure is the way it is, but without this and with
the changes from this bug we start navigating away which means we try to reach
mozilla.org, which is forbidden in automation since non-local connections are,
causing the failures that got me backed out in comment 25.

Backout by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8fda1a51f2c9
Backed out changeset 60e865d21df5 for Crashtest failures on dom/l10n/tests/mochitest/dom_localization/test_overlay.html. CLOSED TREE
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ede6a6b57500
Prevent navigating away in test_overlay.html. r=zbraniecki
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/63a3ab2bf813
Document::GetContainer shouldn't be virtual. r=smaug
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/fc09b999be43
Allow navigating when there's no pres context. r=smaug
https://hg.mozilla.org/integration/autoland/rev/00823046548a
Remove nsILinkHandler. r=smaug
https://hg.mozilla.org/integration/autoland/rev/90ce604ee5c5
Simplify CheckHandleEventForAnchorsPreconditions. r=smaug
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/a5df0d208c25
Remove now-unneeded workarounds in cross-origin navigation tests. r=smaug
Flags: needinfo?(emilio)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/953a42638777
Port bug 1218456 - Remove include of nsILinkHandler.h from C-C. rs=bustage-fix DONTBUILD
Blocks: 1564980

Comment on attachment 9076791 [details]
Bug 1218456 - Allow navigating when there's no pres context. r=smaug

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is causing webcompat issues on Fennec as well as the ESR build, and porting it seems to involve low effort and low risk.
  • User impact if declined: Web compat issues such as https://webcompat.com/issues/42666 on the ESR branch.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): According to Emilio the risk is low and the patches apply cleanly to ESR, and I'm inclined to agree given that we have seen no fallout from this so far since it landed for Firefox 70.
  • String or UUID changes made by this patch:
Attachment #9076791 - Flags: approval-mozilla-esr68?
Attachment #9076792 - Flags: approval-mozilla-esr68?
Attachment #9076793 - Flags: approval-mozilla-esr68?
Attachment #9076794 - Flags: approval-mozilla-esr68?
Attachment #9076817 - Flags: approval-mozilla-esr68?
Attachment #9076909 - Flags: approval-mozilla-esr68?

Comment on attachment 9076909 [details]
Bug 1218456 - Prevent navigating away in test_overlay.html. r=zbraniecki

This patch touches a test that isn't on ESR. Other than this the rest apply cleanly.

Attachment #9076909 - Flags: approval-mozilla-esr68?

Comment on attachment 9076791 [details]
Bug 1218456 - Allow navigating when there's no pres context. r=smaug

Low risk and high webcompat impact, uplift pproved for ESR 68.3, thanks.

Attachment #9076791 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9076792 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9076793 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9076794 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9076817 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: