372098 - HTML links with target="" should use the base target

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:
- Go to http://tuinbouw.startkabel.nl/
- Click on a link like Floranet (it should open the link in a new tab)
- Watch the location bar
- Click Reload

Result: page returns to http://tuinbouw.startkabel.nl/
It does the same when you click Back and it also listens to Forward.
This happens on all daughter pages of Startkabel. Not sure if this is an evangelism bug. No reduced testcase.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Problem is that it should load the page in a new tab but it doesn't, and loads the page in the same tab instead without updating the location bar.

Comment 2

[Jonathan Watt [:jwatt]]

What's happening is that the links have target="" set on them, and in bug 334587 I seem to have changed the behavior so that that loads the linked document in the same window instead of a new one.

Note that the document with the Floranet (etc.) links is in a frame, so the location bar and Reload/Back/Forward behavior is expected given the change in target behavior.

Comment 3

[Jonathan Watt [:jwatt]]

Sorry, my statement about the target attribute was wrong. Setting target="" is the same as not specifying a target at all. What's happening is that the page has <base target="_blank">, so it appears the problem is we don't properly detect the base target if the target isn't set on the link.

Comment 4

[Jonathan Watt [:jwatt]]

This problem only occurs when the target attribute on the link is set but empty.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

We used to use GetAttr in nsGenericHTMLElement::PostHandleEventForAnchors to get the target attribute and if that was empty get the base target. Bug 334587 then introduced GetLinkTarget to fetch the actually target that should be used for the element. The bug is that GetLinkTarget only fetches the base target if the target attribute is not set. It should also fetch the base target if the target attribute is set but empty.

Comment 6

[Jonathan Watt [:jwatt]]

While looking at this, I noticed that the value we return for HTMLLinkElement::target

  http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-84183095

is different to what IE6 returns. We seem to try (but in some cases fail) to return the "target" that will actually be _used_ when the link is activated. For example, if the 'target' attribute is not set, the value of the target property will be the base target. On the other hand if the 'target' attribute is set but empty, value of the target property will be the empty string, even though we (and IE) will use the base target in this case. IE is consistent. It seems to always returns the value of the 'target' attribute regardless of the target that will actually be used. Perhaps we should too, especially since our current behavior is inconsistent.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I'd like to see (mochitest) testcases for <a>, <area> and <link> elements.

Comment 8

[Jonathan Watt [:jwatt]]

After sitting down to properly focus on this bug it took me about twenty minutes to go through the code, write and test the patch. So far to get a damn Mochitest test I've spent almost two full days finding out what Mochitest is, how you use it, writing up my findings on MDC so no one else has to go through all that, figuring out how to write a test for this particular bug, debugging the damn thing, and now I find bug 374507 seems to block me anyway. In future I think anyone requiring tests to be written before giving review should make sure there's adequate documented on how to create the said tests.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Sorry :)
Though I think we all just should get used to writing testcases.
For example this morning one mochitest testcase (chrome test to be 
exact) showed immediately a regression and fixing it was fast. 
Without the testcase someone might have noticed the
problem much later.
But yes, the documentation how to write tests should be better!

Comment 10

[Jonathan Watt [:jwatt]]

No worries, I'm just old and grumpy. :-P I should have known better than to think I could knock up a test for a new and unfamiliar suite on my Sunday afternoon. It just gets frustrating when you need to code and something like this get in the way by dragging on into the night and into the next day.

I absolutely agree adding tests to the automated test suites should be part of our regular routine. It rocks that we now have something like Mochitest. The learning curve should be as shallow as possible though, and developing a test should use up as little of our time as possible. Especially if we're going to expect new contributors to do this when submitting their first patches. The barrier to entry to contributing to Mozilla is high enough as it is.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch plus mochitest

Comment 12

[Robert Sayre]

(In reply to comment #11)
> Created an attachment (id=259041) [details]
> patch plus mochitest

Nice. feel free to donate sendMouseEvent to 

http://lxr.mozilla.org/seamonkey/source/testing/mochitest/tests/SimpleTest/EventUtils.js

which only does keys right now.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 259041 [details] [diff] [review]
patch plus mochitest

> nsHTMLAnchorElement::GetLinkTarget(nsAString& aTarget)
> {
>-  GetTarget(aTarget);
>+  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::target, aTarget) ||
>+      aTarget.IsEmpty()) {
>+    GetBaseTarget(aTarget);
>+  }

No need to test the return value of GetAttr. Move it out of the |if| and only test aTarget.IsEmpty()

> nsHTMLAreaElement::GetLinkTarget(nsAString& aTarget)
> {
>-  GetTarget(aTarget);
>+  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::target, aTarget) ||
>+      aTarget.IsEmpty()) {
>+    GetBaseTarget(aTarget);
>+  }
> }

Same here

> nsHTMLLinkElement::GetLinkTarget(nsAString& aTarget)
> {
>-  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::target, aTarget)) {
>+  if (!GetAttr(kNameSpaceID_None, nsGkAtoms::target, aTarget) ||
>+      aTarget.IsEmpty()) {
>     GetBaseTarget(aTarget);
>   }

And here.

>+function callback(tag)
>+{
>+  if (tag == 'a') {
>+    send_click_to(document.getElementById('link'));
>+    return;
>+  }
>+  if (tag == 'link') {
>+    send_click_to(document.getElementById('area'));
>+    return;
>+  }
>+  if (tag == 'area') {
>+    ok(true, "Elements 'a', 'link', and 'area' tested.");
>+    SimpleTest.finish();
>+    return;
>+  }
>+  throw new Error("Eh??? We only test the 'a', 'link' and 'area' elements.");
>+}

It's not too good to have the tests cascade like this and only have a single check at the end. If we do something wrong so that we shortcut to the last callback right away we'll still pass the test.

A better solution would be to have three variables set to false to start off and once all tests have finished have a test to make sure that they're all true.

I wish that mochitest had a callback that allowed you to specify how many ok-calls were expected and that marked us as failing if we didn't make the appropriate number. That's for a different bug of course.

sr=me with the other things fixed.

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: patch checked in

Address review comments and put a generalized version of sendMouseEvent into EventUtils.js.