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.
Created attachment 241344 [details] Patch Attribute text in HTMLAnchorElement should behave as per specification.
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 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
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 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
Created attachment 241417 [details] Patch-Updated-Review2 Updated patch to add SetterRaisesException.
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 **
(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.
(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?
(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.
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 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.
Created attachment 241529 [details] Patch-Updated-Review4 updated change log as per comments, build is passing now.
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.
Dear All, Patch is now built fine on all bots. waiting for review/commit. Thanks
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 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 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.
Created attachment 241687 [details] Patch-Updated-Review5 updated the patch as per review comments.
Comment on attachment 241687 [details] Patch-Updated-Review5 Clearing flags on attachment: 241687 Committed r176169: <http://trac.webkit.org/changeset/176169>
All reviewed patches have been landed. Closing bug.
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).
Re-opened since this is blocked by bug 138797
Actually, I was confused - only a Gtk merge of this patch was taken in the regression range. Will re-land now.
Created attachment 241727 [details] patch for re-landing
Comment on attachment 241727 [details] patch for re-landing Clearing flags on attachment: 241727 Committed r176213: <http://trac.webkit.org/changeset/176213>