Links don't work if the text has formatting inside an <a> tag #4236

lizzard · 2014-12-05T05:54:20Z

In this test case, the first two links work, and the last link works, but the third one with the <b> tag inside the tag doesn't work.

<html>
    <head></head>
    <body>
        <a href="http://google.com"> without bold tag</a>
        <b><a href="http://google.com">with bold tag outside a tag</a></b>
        <a href="http://google.com"><b>b tag inside a tag</b> Text outside b tag</a>

    </body>
</html>

jdm · 2014-12-05T16:00:14Z

This is a good testcase for the design of #4002; thanks!

untitaker · 2014-12-09T18:58:52Z

#4002 seems merged, can this be closed?

jdm · 2014-12-09T19:10:27Z

Have we tested whether this case works now?

narfg · 2014-12-10T08:53:51Z

The bold part of the last link still doesn't work.

eddyb · 2014-12-10T09:17:52Z

Do we bubble up click events at all?

jdm · 2014-12-10T15:19:56Z

Paging @Manishearth. The concept of "nearest activatable element" that he implemented should be relevant here.

jdm · 2014-12-10T15:20:58Z

Oh, it looks like we haven't hooked up link clicking to the new activation code; it still uses actual click events. We should correct that.

Manishearth · 2014-12-10T15:23:08Z

Yeah, it's on my todo (or, hopefully-get-Ms2ger-todo) list. Ms2ger had a partial implementation but it won't fit cleanly with Activatable so I guess I'll need to rewrite.

If someone else wants to pick it up, feel free -- I can mentor 😄

Manishearth · 2014-12-15T01:53:31Z

Spec link: https://html.spec.whatwg.org/multipage/semantics.html#the-a-element:activation-behavior

You can see an example usage of Activatable here

implicit_submission(), pre_click_activation(), and canceled_activation() can be empty (and inlined) for this case -- you only need to write code for as_element() and activation_behavior(). Perhaps when we support :active for links we might want to fill some of the other methods in.

You'll also need to add to the vtable in ActivationElementHelpers::as_maybe_activatable

jdm · 2014-12-31T16:13:15Z

@shinglyu This would be a valuable issue to work on if you're interested :)

shinglyu · 2014-12-31T16:21:02Z

Ah thanks @jdm , I'll work on it.

shinglyu · 2015-01-19T15:09:11Z

Hi @Manishearth , I read the spec and tried to find the relevant code. I wonder where is the code that handles <a/> click in the current implementation?

Also, can you explain what "You'll also need to add to the vtable in ActivationElementHelpers::as_maybe_activatable" means? Do you mean I need to add HtmlLinkElement to the match in https://github.com/servo/servo/blob/master/components/script/dom/element.rs#L1204 ?

Thank you.

jdm · 2015-01-19T15:14:50Z

http://mxr.mozilla.org/servo/source/components/script/dom/htmlanchorelement.rs#59 is the current implementation. And the answer to the second question is that you need to add HTMLAnchorElement to the match in https://github.com/servo/servo/blob/master/components/script/dom/element.rs#L1437

Manishearth · 2015-01-19T15:52:32Z

Currently a clicks are under handle_event as Josh linked to; this is wrong -- most of this (everything from the spec link in my comment above) should be in an Activatable impl.

shinglyu · 2015-01-27T13:11:30Z

Is there any relevant automation test for this? Or do I have to manually click through the links to test it? Thanks.

jdm · 2015-01-27T13:25:47Z

I think if you make a test like http://mxr.mozilla.org/servo/source/tests/content/test_click_prevent.html?force=1 and test that the click handler is called, that might work.

Manishearth · 2015-01-27T15:35:37Z

Activation is a bit harder to test for nested elements since click() (synthetic activation) is not exactly the same as actually clicking. I suggest mostly relying on manual tests though if you find something that works that's automated, that would be great!

shinglyu · 2015-02-03T14:53:28Z

Just moved the click handler code into the activation_behavior function: https://github.com/shinglyu/servo/compare/bug4236
Now all the links works.
I'll fill in those checks as in the spec. Is there anything we can't handle yet and should be skipped for now?

Manishearth · 2015-02-03T15:04:21Z

Ah, nice. Make sure you include the is_instance_activatable check for <a> as well (https://github.com/servo/servo/blob/master/components/script/dom/element.rs#L1450), perhaps moving the check outside of the match block where it belongs.

shinglyu · 2015-02-04T16:05:29Z

Hmm.. I added something like this: https://github.com/shinglyu/servo/compare/bug4236#diff-9082d52f79e121265b7ddf64d11fe044R1468
But the compiler said 1463:46 error: the type of this value must be known in this context. Maybe I just create a function for this is_instance_activatable check and call it in every match branch?

Manishearth · 2015-02-04T17:22:47Z

Oh, that's because you've not casted everything properly. You should be casting within the match itself for each arm, and the match should produce an Option<&'a Activatable + 'a> or something

jdm · 2015-02-04T17:25:55Z

Yes, each arm of the match needs to produce an identical type. Right now that's not happening.

shinglyu · 2015-02-08T10:27:21Z

The previous problem is solved, I manually tested it and the <a> tags work.

But I have some problem understanding step 1, 2 and 3 in anchor element's activation behaviour, can I create a pull request for my current progress, and deal with these steps in a follow-up bug?

Step 1: Don't really know how to check if the node document is fully active, I guess it's like button element's is_instance_activitable code? But what does the disabled state means?

Step 2: There are too many technical terms I don't understand yet, I may need some time to understand them and figure out how to turn them into Rust code.

Step 3: Seems irrelevant to the test cases in the first comment. It seems like a separate feature and I can implement it in a follow up issue.

Manishearth · 2015-02-08T11:11:45Z

TL;DR: Skip all three if you want; leave comments/file followup issues. The third one is something you might want to try (whether in the same PR or another).

Just some explanation of the specs/code to get you more familiar:

Step 1

is_instance_activatable() checks if the HTMLButtonElement instance is activatable. The class is in general activatable, but disabled elements are not, so we need the type-based-check followed by the method call. This doesn't have anything to do with the active browsing context.

I don't think we have a way of checking document activity yet; at least my implementation for <input> just has a comment in it. You can do the same, I guess 😄

Step 2

Okay, this one's a doozy. However, we don't support tabs and all yet, and frame support is pretty flaky, so working this out IMO isn't too important. Again, I suggest you skip this and leave a comment (file a followup maybe?). I'll still explain it in part so that you'll be able to understand such things in the future :)

It's basically talking about handling the target attribute, which can be _blank (popup), _self (same window, default), _top (toplevel parent frame), _parent (direct parent frame), or the name of another frame. These get handled differently depending on the attributes of the link, the containing frame, and also the user settings (eg popup blocking behavior). It gets quite complex, so I think we should work on abstracting over the terms in the webpage loading spec separately.

Step 3

This one can actually be implemented, though I personally don't mind if you skip it for now. This has to do with image maps -- these are images where clicking in different places is supposed to have different behavior.

Just in case you need help with it, here is the same algorithm deconstructed with links to relevant code you might want to use. Try not to use this, but it will be useful in case you get stuck 😄

If the a element contains an <img ismap>, which was the target of the click event (look at the target of the mouseevent, cast it to a Node, check that the a element is one of the ancestors(), ...
..., and the click was a real click (i.e. from the user, not script-generated) ...
... then calculate the offset from the click event (MouseEvent has some fields which you can access for that, and you can do something similar to getClientBoundingRects to get the image position).
Then you just append ?x,y to it, given x and y (which should be zero in case of a synthetic activation), and navigate as usual

To implement this you'll need to add two new arguments to activation_behavior():

Whether or not it was a "real" click -- authentic_click_activation() will set this, synthetic_click_activation() won't, and IIRC we don't have any other callers.
The MouseEvent behind the click.

Ms2ger · 2015-02-08T14:34:14Z

I'd leave all three alone for the purpose of fixing this issue; however, please do leave comments along the lines of "// Step 1: document not fully active". Bonus points for filing issues about those steps.

shinglyu · 2015-02-09T03:05:40Z

Thanks @Manishearth for the great tutorial!
@Ms2ger: I'll file the issues, thanks.


        auto merge of #4870 : shinglyu/servo/bug4236, r=jdm

Fixed bug #4236

jdm added A-content/dom E-less easy labels Dec 10, 2014

This was referenced Feb 9, 2015

Bug 4236: Moved anchorelement click event to activation behaviour #4870

Merged

Check if the node document is fully active in AnchorElement's activation_behavior #4871

Closed

This was referenced Feb 9, 2015

Handle specific browsing context in AnchorElement's activation behaviour #4872

Open

Support the image map processing for <img ismap/> inside an <a/> #4873

Closed

bors-servo pushed a commit that referenced this issue Feb 21, 2015

auto merge of #4870 : shinglyu/servo/bug4236, r=jdm

Loading status checks…

95cc170

Fixed bug #4236

Ms2ger mentioned this issue Jul 24, 2015

Jumpy styles #6735

Open

Ms2ger closed this Jul 24, 2015

servo / servo

Links don't work if the text has formatting inside an <a> tag #4236

Links don't work if the text has formatting inside an <a> tag #4236

lizzard commented Dec 5, 2014

jdm commented Dec 5, 2014

untitaker commented Dec 9, 2014

jdm commented Dec 9, 2014

narfg commented Dec 10, 2014

eddyb commented Dec 10, 2014

jdm commented Dec 10, 2014

jdm commented Dec 10, 2014

Manishearth commented Dec 10, 2014

Manishearth commented Dec 15, 2014

jdm commented Dec 31, 2014

shinglyu commented Dec 31, 2014

shinglyu commented Jan 19, 2015

jdm commented Jan 19, 2015

Manishearth commented Jan 19, 2015

shinglyu commented Jan 27, 2015

jdm commented Jan 27, 2015

Manishearth commented Jan 27, 2015

shinglyu commented Feb 3, 2015

Manishearth commented Feb 3, 2015

shinglyu commented Feb 4, 2015

Manishearth commented Feb 4, 2015

jdm commented Feb 4, 2015

shinglyu commented Feb 8, 2015

Manishearth commented Feb 8, 2015

Ms2ger commented Feb 8, 2015

shinglyu commented Feb 9, 2015

servo / servo

Join GitHub today

Links don't work if the text has formatting inside an <a> tag #4236

Links don't work if the text has formatting inside an <a> tag #4236

Comments

lizzard commented Dec 5, 2014

jdm commented Dec 5, 2014

untitaker commented Dec 9, 2014

jdm commented Dec 9, 2014

narfg commented Dec 10, 2014

eddyb commented Dec 10, 2014

jdm commented Dec 10, 2014

jdm commented Dec 10, 2014

Manishearth commented Dec 10, 2014

Manishearth commented Dec 15, 2014

jdm commented Dec 31, 2014

shinglyu commented Dec 31, 2014

shinglyu commented Jan 19, 2015

jdm commented Jan 19, 2015

Manishearth commented Jan 19, 2015

shinglyu commented Jan 27, 2015

jdm commented Jan 27, 2015

Manishearth commented Jan 27, 2015

shinglyu commented Feb 3, 2015

Manishearth commented Feb 3, 2015

shinglyu commented Feb 4, 2015

Manishearth commented Feb 4, 2015

jdm commented Feb 4, 2015

shinglyu commented Feb 8, 2015

Manishearth commented Feb 8, 2015

Ms2ger commented Feb 8, 2015

shinglyu commented Feb 9, 2015