Bug 138557 – Attribute text in HTMLAnchorElement should behave as per specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shivakumar J M]

Attribute text should not be readonly in HTMLAnchorElement, it  should behave as per specification: http://www.w3.org/TR/html/text-level-semantics.html#dom-a-text

The text IDL attribute, on getting, must return the same value as the textContent IDL attribute on the element, and on setting, must act as if the textContent IDL attribute on the element had been set to the new value.

[Shivakumar J M]

Created attachment 241344 [details]
Patch

Attribute text in HTMLAnchorElement should behave as per specification.

[Shivakumar J M]

Build failure due to public API removed, but build passed in local machine. So instead of these change, i will try to add setter API for text attribute with new patch, instead of removing the existing text() api and do not use implementedAs=textContent.

[Chris Dumez]

Comment on attachment 241344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241344&action=review

r- since it doesn't build on mac.

> Source/WebCore/ChangeLog:10
> +        Make attribute text in HTMLAnchorElement should behave as per specification http://www.w3.org/TR/html/text-level-semantics.html#dom-a-

"Make attribute text in HTMLAnchorElement should behave" <- Not a correct sentence

> Source/WebCore/ChangeLog:11
> +        text. It should behave as textContent attribute. Also This matches the behavior of Chrome 38.

Please try at least Firefox as well. According to my changelog for https://codereview.chromium.org/263353004, it matches the behavior of Firefox and IE as well but please double check.
s/This/this

[Shivakumar J M]

Created attachment 241410 [details]
Patch-Updated-Review1

Since Build fail occured if we use implementedAs=textContent, so Updated the patch to use new setText() api.

[Chris Dumez]

Comment on attachment 241410 [details]
Patch-Updated-Review1

View in context: https://bugs.webkit.org/attachment.cgi?id=241410&action=review

> Source/WebCore/html/HTMLAnchorElement.cpp:512
> +    ExceptionCode ec;

We should not ignore the Execution code.

> Source/WebCore/html/HTMLAnchorElement.idl:59
> +    attribute DOMString text;

We likely want a [SetterRaisesException] here

[Shivakumar J M]

Created attachment 241417 [details]
Patch-Updated-Review2

Updated patch to add SetterRaisesException.

[Shivakumar J M]

It looks to be some problem in build in CodeGeneratorObjC.pm. https://bugs.webkit.org/show_bug.cgi?id=135860.
Public API change. There are missing public properties and/or methods from the "DOMHTMLAnchorElement" class.
@property (readonly, copy) NSString *text;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
make: *** [DOMHTMLAnchorElement.h] Error 255
make: *** Waiting for unfinished jobs....
Showing first 200 notices only
Command /bin/sh failed with exit code 2

** BUILD FAILED **

[Chris Dumez]

(In reply to comment #7)
> It looks to be some problem in build in CodeGeneratorObjC.pm.
> https://bugs.webkit.org/show_bug.cgi?id=135860.
> Public API change. There are missing public properties and/or methods from
> the "DOMHTMLAnchorElement" class.
> @property (readonly, copy) NSString *text;
> Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
> make: *** [DOMHTMLAnchorElement.h] Error 255
> make: *** Waiting for unfinished jobs....
> Showing first 200 notices only
> Command /bin/sh failed with exit code 2
> 
> ** BUILD FAILED **

I don't know anything about the ObjC bindings so someone else will need to take over. I think you'll need to update DOMHTMLAnchorElement in Source/WebCore/bindings/objc/PublicDOMInterfaces.h and remove "readonly" form text. However, it seems we do API versioning so it may not be as easy. Dropping readonly seems like a backward compatible API change though so I am not sure it is a problem. As I said, someone knowledgeable about ObjC bindings should comment on this.

[Shivakumar J M]

(In reply to comment #8)
> (In reply to comment #7)
> > It looks to be some problem in build in CodeGeneratorObjC.pm.
> > https://bugs.webkit.org/show_bug.cgi?id=135860.
> > Public API change. There are missing public properties and/or methods from
> > the "DOMHTMLAnchorElement" class.
> > @property (readonly, copy) NSString *text;
> > Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
> > make: *** [DOMHTMLAnchorElement.h] Error 255
> > make: *** Waiting for unfinished jobs....
> > Showing first 200 notices only
> > Command /bin/sh failed with exit code 2
> > 
> > ** BUILD FAILED **
> 
> I don't know anything about the ObjC bindings so someone else will need to
> take over. I think you'll need to update DOMHTMLAnchorElement in
> Source/WebCore/bindings/objc/PublicDOMInterfaces.h and remove "readonly"
> form text. However, it seems we do API versioning so it may not be as easy.
> Dropping readonly seems like a backward compatible API change though so I am
> not sure it is a problem. As I said, someone knowledgeable about ObjC
> bindings should comment on this.

Dear Chris,

     Thanks for inputs, i think we need to remove "readonly" from text in PublicDOMInterfaces.h for DOMHTMLAnchorElement. I found some old bug:
https://bugs.webkit.org/show_bug.cgi?id=87154, it has similar change done.
     I will make these change and update the parch. But, as u pointed out, these change might drop backward compatible, so shall we do these change or not?

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > It looks to be some problem in build in CodeGeneratorObjC.pm.
> > > https://bugs.webkit.org/show_bug.cgi?id=135860.
> > > Public API change. There are missing public properties and/or methods from
> > > the "DOMHTMLAnchorElement" class.
> > > @property (readonly, copy) NSString *text;
> > > Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 386.
> > > make: *** [DOMHTMLAnchorElement.h] Error 255
> > > make: *** Waiting for unfinished jobs....
> > > Showing first 200 notices only
> > > Command /bin/sh failed with exit code 2
> > > 
> > > ** BUILD FAILED **
> > 
> > I don't know anything about the ObjC bindings so someone else will need to
> > take over. I think you'll need to update DOMHTMLAnchorElement in
> > Source/WebCore/bindings/objc/PublicDOMInterfaces.h and remove "readonly"
> > form text. However, it seems we do API versioning so it may not be as easy.
> > Dropping readonly seems like a backward compatible API change though so I am
> > not sure it is a problem. As I said, someone knowledgeable about ObjC
> > bindings should comment on this.
> 
> Dear Chris,
> 
>      Thanks for inputs, i think we need to remove "readonly" from text in
> PublicDOMInterfaces.h for DOMHTMLAnchorElement. I found some old bug:
> https://bugs.webkit.org/show_bug.cgi?id=87154, it has similar change done.
>      I will make these change and update the parch. But, as u pointed out,
> these change might drop backward compatible, so shall we do these change or
> not?

Yes, since the change suggested is trivial (and no one else commented on this yet), I would make the change to make mac ews bots happy.

[Shivakumar J M]

Created attachment 241469 [details]
Patch-Updated-Review3

Updated the patch to make build pass by removing readonly for text attribute in DOMHTMLAnchor from PublicDOMInterfaces.h.

[Chris Dumez]

Comment on attachment 241469 [details]
Patch-Updated-Review3

View in context: https://bugs.webkit.org/attachment.cgi?id=241469&action=review

> Source/WebCore/ChangeLog:8
> +        Test: fast/dom/HTMLAnchorElement/anchor-text-attribute.html

nit: this normally comes *after* the changelog, not before.

[Shivakumar J M]

Created attachment 241529 [details]
Patch-Updated-Review4

updated change log as per comments, build is passing now.

[Shivakumar J M]

Created attachment 241564 [details]
Patch-Updated-Review5

uploading the same patch again, since same patch passed build for Win earlier, so just triggering build again with same patch.

[Shivakumar J M]

Dear All,

     Patch is now built fine on all bots. waiting for review/commit.

Thanks

[Chris Dumez]

Comment on attachment 241564 [details]
Patch-Updated-Review5

The change looks good to me (and it is aligning WebKit with the specification and other major engines). However, since it is touching the ObjC public API, I will let someone else review this.

[Darin Adler]

Comment on attachment 241564 [details]
Patch-Updated-Review5

View in context: https://bugs.webkit.org/attachment.cgi?id=241564&action=review

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:355
> -@property (readonly, copy) NSString *text WEBKIT_AVAILABLE_MAC(10_5);
> +@property (copy) NSString *text WEBKIT_AVAILABLE_MAC(10_5);

This change won’t be needed if you make the change to the IDL I suggest below.

We should not make the change. If do we make this change, then this incorrectly claims that apps can set the text attribute when compiled against the version of WebKit on OS X 10.5, which is not true.

> Source/WebCore/html/HTMLAnchorElement.cpp:507
> -    return innerText();
> +    return textContent();

Which of the tests cover this change? Or do innerText and textContent behave identically?

> Source/WebCore/html/HTMLAnchorElement.idl:59
> -    readonly attribute DOMString text;
> +    [SetterRaisesException] attribute DOMString text;

The conservative way to keep compatibility with Objective-C API would be to write this:

    #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
        readonly attribute DOMString text;
    #else
        [SetterRaisesException] attribute DOMString text;
    #endif

We could even merge this with the #if statement above that seems to be doing something similar for other attributes.

[Chris Dumez]

Comment on attachment 241564 [details]
Patch-Updated-Review5

View in context: https://bugs.webkit.org/attachment.cgi?id=241564&action=review

>> Source/WebCore/html/HTMLAnchorElement.cpp:507
>> +    return textContent();
> 
> Which of the tests cover this change? Or do innerText and textContent behave identically?

This is the last one: '<a>a<br>b</a>'. textContent does not convert BRs to newlines.

> LayoutTests/fast/dom/HTMLAnchorElement/anchor-text-attribute.html:28
> +shouldBeEqualToString('a.text', 'ab');

This would return 'a\nb' if it did not behave as textContent.

[Shivakumar J M]

Created attachment 241687 [details]
Patch-Updated-Review5

updated the patch as per review comments.

[WebKit Commit Bot]

Comment on attachment 241687 [details]
Patch-Updated-Review5

Clearing flags on attachment: 241687

Committed r176169: <http://trac.webkit.org/changeset/176169>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This seems to have broken <https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r176199%20(537)/svg/custom/anchor-on-use-diffs.html> on Yosemite Debug testers only.

I'm going to roll out to confirm, and will land again if it's something else (unfortunately, the regression range is somewhat long).

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138797

[Alexey Proskuryakov]

Actually, I was confused - only a Gtk merge of this patch was taken in the regression range. Will re-land now.

[Alexey Proskuryakov]

Created attachment 241727 [details]
patch for re-landing

[WebKit Commit Bot]

Comment on attachment 241727 [details]
patch for re-landing

Clearing flags on attachment: 241727

Committed r176213: <http://trac.webkit.org/changeset/176213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.