Bug 138557

Summary: Attribute text in HTMLAnchorElement should behave as per specification
Product: WebKit Reporter: Shivakumar J M <shiva.jm>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, cdumez, commit-queue, darin, dbates, gyuyoung.kim, jer.noble, rniwa, sam, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 138797    
Bug Blocks:    
Attachments:
Description Flags
Patch
cdumez: review-, cdumez: commit-queue-
Patch-Updated-Review1
cdumez: review-, cdumez: commit-queue-
Patch-Updated-Review2
none
Patch-Updated-Review3
none
Patch-Updated-Review4
none
Patch-Updated-Review5
none
Patch-Updated-Review5
none
patch for re-landing none

Description Shivakumar J M 2014-11-09 23:02:56 PST
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.
Comment 1 Shivakumar J M 2014-11-11 02:12:47 PST
Created attachment 241344 [details]
Patch

Attribute text in HTMLAnchorElement should behave as per specification.
Comment 2 Shivakumar J M 2014-11-11 04:01:49 PST
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.
Comment 3 Chris Dumez 2014-11-11 09:24:03 PST
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
Comment 4 Shivakumar J M 2014-11-11 19:47:52 PST
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.
Comment 5 Chris Dumez 2014-11-11 19:54:39 PST
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
Comment 6 Shivakumar J M 2014-11-12 00:44:32 PST
Created attachment 241417 [details]
Patch-Updated-Review2

Updated patch to add SetterRaisesException.
Comment 7 Shivakumar J M 2014-11-12 01:25:15 PST
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 **
Comment 8 Chris Dumez 2014-11-12 09:34:39 PST
(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.
Comment 9 Shivakumar J M 2014-11-12 21:56:57 PST
(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?
Comment 10 Chris Dumez 2014-11-12 22:03:00 PST
(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.
Comment 11 Shivakumar J M 2014-11-12 22:30:39 PST
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.
Comment 12 Chris Dumez 2014-11-12 22:32:41 PST
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.
Comment 13 Shivakumar J M 2014-11-13 19:25:18 PST
Created attachment 241529 [details]
Patch-Updated-Review4

updated change log as per comments, build is passing now.
Comment 14 Shivakumar J M 2014-11-14 00:34:38 PST
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.
Comment 15 Shivakumar J M 2014-11-14 07:10:24 PST
Dear All,

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

Thanks
Comment 16 Chris Dumez 2014-11-14 09:59:37 PST
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.
Comment 17 Darin Adler 2014-11-14 17:26:16 PST
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.
Comment 18 Chris Dumez 2014-11-14 17:31:53 PST
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.
Comment 19 Shivakumar J M 2014-11-16 20:25:53 PST
Created attachment 241687 [details]
Patch-Updated-Review5

updated the patch as per review comments.
Comment 20 WebKit Commit Bot 2014-11-16 21:14:42 PST
Comment on attachment 241687 [details]
Patch-Updated-Review5

Clearing flags on attachment: 241687

Committed r176169: <http://trac.webkit.org/changeset/176169>
Comment 21 WebKit Commit Bot 2014-11-16 21:14:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Alexey Proskuryakov 2014-11-17 10:42:26 PST
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).
Comment 23 WebKit Commit Bot 2014-11-17 10:44:47 PST
Re-opened since this is blocked by bug 138797
Comment 24 Alexey Proskuryakov 2014-11-17 11:10:07 PST
Actually, I was confused - only a Gtk merge of this patch was taken in the regression range. Will re-land now.
Comment 25 Alexey Proskuryakov 2014-11-17 11:11:44 PST
Created attachment 241727 [details]
patch for re-landing
Comment 26 WebKit Commit Bot 2014-11-17 11:24:52 PST
Comment on attachment 241727 [details]
patch for re-landing

Clearing flags on attachment: 241727

Committed r176213: <http://trac.webkit.org/changeset/176213>
Comment 27 WebKit Commit Bot 2014-11-17 11:25:02 PST
All reviewed patches have been landed.  Closing bug.