113310 - Adding/Removing href attribute on <a> node does not update appearance

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[kinmoz]

This is an offshoot from bug 108196, which composer may have to work around 
until this bug gets fixed ...

It seems that adding or removing the href attribute of an <a> node that is 
already in the content tree, does not trigger the layout/style code necessary to 
update it's appearance.

That is if an <a> node in the tree doesn't have an href attribute, all of the 
content underneath it does not look like a link on screen. Adding the href 
attribute should then make it look like a link, and removing it should restore 
it back to it's default look.

Comment 1

[kinmoz]

Attachment: JS Test Case

This case allows you to dynamically add/remove the href attribute of an <a>
node in the content tree.

Comment 2

[Neil Pryde]

This works, just to let you know:
onclick="document.getElementById('atag').href='http://www.mozilla.org'">

Comment 3

[Charles Manske]

Any chance of this being scheduled for fixing?

Comment 4

[Marc Attinasi]

I'll take a guess and suggest that we can probably fix this by indicating that a
change to the href attribute should cause a reframe of the element. I'll try it,
should be easy. If not, then I'll update the bug so you can do the workaround.

Comment 5

[Marc Attinasi]

Hmm. That did not fix it. I think there is some Style System caching going on
here - over to Style System to investigate.

Comment 6

[Håkan Waara]

Just FYI: bug 108196 is now a very frequent duplicate ... is the milestone on
this one realistic?  Just wanted to let you know.

Comment 7

[Akkana Peck]

Bug 108196, which depends on this, is EDITORBASE, so this needs to be on the
EDITORBASE radar as well.

Comment 8

[Akkana Peck]

Bug 108196 was just marked QAHP-top, so this one has to be too.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: second JS test case

This bug is not specific to anchors ; check second test case.

Comment 10

[Kathleen :Brade]

Can someone pick this up if Pierre isn't going to get to it soon?
DBaron?  Attinasi?

Comment 11

[Charles Manske]

Daniel: Can you look into this?

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Is the problem here that the restyle doesn't happen, or is it just that we're
not clearing the cached link state?  Perhaps nsHTMLAnchorElement,
nsHTMLAreaElement, and nsHTMLLinkElement (i.e., the elements that implement
nsILink, at least last I checked) need to override some additional functions to
ensure that they clear the cached link state.

(The other possibility is that we remove this caching and put the info into
RuleProcessorData -- now that the RuleProcessorData is constructed in
WalkRuleProcessors/FileRules (I think), this shouldn't be as much of a
performance hit as it used to be, although it would still be a performance hit
for all dynamic reresolution of style, so I'm inclined to leave the optimization
in.)

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 65617 [details]
second JS test case

Marking this testcase obsolete since it works correctly when the missing ' and
) are added.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Right, seen from debug, it seems that we never clear the cached link state.

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

accepting bug

Comment 16

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v1.0

David : can you review the patch please ?

Comment 17

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v2.0

Comment 18

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

nominating nsbeta1.
editor folks : if this gets r= and sr= and I am not available, feel free to
check it in for me!

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 70076 [details] [diff] [review]
patch v2.0

r=peterv

Comment 20

[Charles Manske]

Note that bug 108196, which this blocks, is "EDITORBASE+, QAHP-top" and nsbeta1+.

Comment 21

[Johnny Stenback (:jst)]

This logic belongs in ::SetAttr(nsINodeInfo *, ...) and ::UnsetAttr() in
nsHTMLAnchorElement, not in the DOM specific attribute methods. If you move the
logic there you can continue to use the macro's for forwarding the DOM methods
and simply override those methods, and that'll do the right thing wrt
case-sensitivity n' all too, w/o ending up calling NormalizeAttrString() twice
for every attribute set. You should also make sure that you only update the link
state if the attribute that's being set is "href" w/o a prefix, to do that check
when you have a nsINodeInfo pointer handy, simply do:

  if (aNodeInfo->Equals(nsHTMLAtoms::href, kNameSpaceID_None)) {
    ...
  }

This would be less code and faster, and also more generic so all attribute sets
would be caught (hmm, except the ones that are done directly with
SetHTMLAttribute(), but let's not worry about that for now).

Fix that, and attach a patch and I'll sr...

Comment 22

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 23

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v3.0

Comment 24

[Johnny Stenback (:jst)]

Hmm, I had a look at the nsGenericHTMLElement code and ::SetAttr(nsINodeInfo *,
...) ends up calling ::SetAttr(PRUint32, ...), so you only need to override the
one that takes the namespace ID, local name, ... arguments, not the one that
takes the nsINodeInfo * argument.

Fix that, and you'll have sr=jst

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 70500 [details] [diff] [review]
patch v3.0

Ok, talked to glazou about this on #mozilla, and we do need to override both
methods since silly C++ can't cope if we don't. This means that the ::SetAttr()
method that takes an nsINodeInfo* should *only* call the base class' method,
and life would be good.

sr=jst with that little change (i.e. remove everything but the last line in
that SetAttr() method).

Comment 26

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: patch v3.1

in answer to irc discussion with jst

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz for 0.9.9

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Shouldn't you fix AREA and LINK as well?

Comment 29

[Johnny Stenback (:jst)]

The same patch should apply more or less directly to those classes
(nsHTMLAreaElement and nsHTMLLinkElement) as well.

Comment 30

[Syd Logan]

EDITORBASE-, but leaving nsbeta1+ on it. Is the patch going to land soon?

Comment 31

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in last friday by jfrancis

Comment 32

[Charles Manske]

*** Bug 108196 has been marked as a duplicate of this bug. ***

Comment 33

[sujay]

Petersen, please verify..thanks.

Comment 34

[Charles Manske]

The fact that the test for bug 108196 works verifies this.