WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
65428
dir=auto needs to work on value of input and textarea elements
https://bugs.webkit.org/show_bug.cgi?id=65428
Summary
dir=auto needs to work on value of input and textarea elements
Aharon (Vladimir) Lanin
Reported
2011-07-31 01:15:39 PDT
dir=auto as currently implemented in WebKit does not do anything useful for input elements, and only works on a textarea's initial value, without changing as the user modifies it. This is because dir=auto currently looks at the element's descendant text nodes. The HTML5 spec (
http://dev.w3.org/html5/spec/Overview.html#the-dir-attribute
) needs to be changed in this regard. Filed bug
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482
on HTML5, asking that the spec be changed such that: - For input and textarea elements with dir=auto, the directionality should be determined not by the element's descendants, but by the element's current value, and should change as the latter changes. - For other elements, the text node descendants under a descendant textarea element should be excluded, just like those in script and style elements.
Attachments
Patch.
(12.05 KB, patch)
2011-08-01 16:06 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch.
(16.10 KB, patch)
2011-08-02 15:32 PDT
,
Yael
darin
: review+
Details
Formatted Diff
Diff
Patch.
(16.25 KB, patch)
2011-08-05 07:10 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yael
Comment 1
2011-08-01 16:06:01 PDT
Created
attachment 102584
[details]
Patch. Changed the directionality algorithm to evaluate the value of input elements and textarea elements when they have the attribute dir-auto Set. Also skip these elements when evaluating the directionality of their ancestors. HTMLTextAreaElement::childrenChanged is not called when a user types into the textarea, so call calculateAndAdjustDirectionality() explicitly from HTMLTextAreaElement::subtreeHasChanged().
Ryosuke Niwa
Comment 2
2011-08-02 12:27:08 PDT
Comment on
attachment 102584
[details]
Patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=102584&action=review
> Source/WebCore/html/HTMLElement.cpp:887 > + if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) || (node->isElementNode() && toElement(node)->isTextFormControl())) {
Maybe split into two lines?
> Source/WebCore/html/HTMLTextAreaElement.cpp:259 > + // When typing in a textarea, childrenChanged is not called, so we need to force the directionality check. > + calculateAndAdjustDirectionality();
Should we also call this in setValue and when "dir" attribute is changed? e.g. dir attribute is removed and we need to figure out new directionality.
> LayoutTests/fast/dom/HTMLElement/attr-dir-auto-text-form-control-expected.txt:9 > +PASS document.defaultView.getComputedStyle(el, null).getPropertyValue('border-right-color') is 'rgb(255, 0, 0)'
This is a very clever way of testing!
Yael
Comment 3
2011-08-02 14:41:59 PDT
(In reply to
comment #2
)
> (From update of
attachment 102584
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=102584&action=review
>
Thank you for the review :)
> > Source/WebCore/html/HTMLElement.cpp:887 > > + if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) || (node->isElementNode() && toElement(node)->isTextFormControl())) { > > Maybe split into two lines? >
ok
> > Source/WebCore/html/HTMLTextAreaElement.cpp:259 > > + // When typing in a textarea, childrenChanged is not called, so we need to force the directionality check. > > + calculateAndAdjustDirectionality(); > > Should we also call this in setValue and when "dir" attribute is changed? e.g. dir attribute is removed and we need to figure out new directionality. >
We already handle the case where dir attribute changes in HTMLTextAreaElement, like we handle it for any other HTMLElement so there is no need to explicitly call calculateAndAdjustDirectionality()
> > LayoutTests/fast/dom/HTMLElement/attr-dir-auto-text-form-control-expected.txt:9 > > +PASS document.defaultView.getComputedStyle(el, null).getPropertyValue('border-right-color') is 'rgb(255, 0, 0)' > > This is a very clever way of testing!
thanks :)
Yael
Comment 4
2011-08-02 15:32:34 PDT
Created
attachment 102705
[details]
Patch. Added a test for dynamic changes to dir attribute in text form controls, and added explicit call to calculateAndAdjustDirectionality() from HTMLInputElement::subtreeHasChanged(), with a check that it is called only for text fields. Also broke the line as suggested in
comment #2
.
Darin Adler
Comment 5
2011-08-02 17:53:45 PDT
Comment on
attachment 102705
[details]
Patch. View in context:
https://bugs.webkit.org/attachment.cgi?id=102705&action=review
> Source/WebCore/html/HTMLElement.cpp:876 > + HTMLTextFormControlElement* textElement = toTextFormControl(const_cast<HTMLElement*>(this)); > + if (textElement) {
Would be elegant to put this definition inside the if so it gets scoped.
> Source/WebCore/html/HTMLElement.cpp:881 > + return (textDirection == WTF::Unicode::LeftToRight) ? LTR : RTL;
Is the WTF:: needed here?
> Source/WebCore/html/HTMLElement.cpp:888 > // Skip bdi, script and style elements > - if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag)) { > + if (equalIgnoringCase(node->nodeName(), "bdi") || node->hasTagName(scriptTag) || node->hasTagName(styleTag) > + || (node->isElementNode() && toElement(node)->isTextFormControl())) {
The comment no longer matches the code.
> Source/WebCore/html/HTMLInputElement.cpp:666 > + // When typing in a input field, childrenChanged is not called, so we need to force the directionality check.
should be "an input field"
Yael
Comment 6
2011-08-04 17:48:30 PDT
rniwa, based on Hixie's comment at
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482#c1
, are you ok with committing this?
Ryosuke Niwa
Comment 7
2011-08-04 17:53:03 PDT
(In reply to
comment #6
)
> rniwa, based on Hixie's comment at
http://www.w3.org/Bugs/Public/show_bug.cgi?id=13482#c1
, are you ok with committing this?
Yes. After all, Darin has already r+ed it :)
Yael
Comment 8
2011-08-05 07:10:19 PDT
Created
attachment 103069
[details]
Patch. Addressing
comment #5
.
WebKit Review Bot
Comment 9
2011-08-05 07:36:48 PDT
Comment on
attachment 103069
[details]
Patch. Clearing flags on attachment: 103069 Committed
r92480
: <
http://trac.webkit.org/changeset/92480
>
WebKit Review Bot
Comment 10
2011-08-05 07:36:53 PDT
All reviewed patches have been landed. Closing bug.
Balazs Kelemen
Comment 11
2011-08-05 08:46:56 PDT
Seems like it broke the Mac and the 64bit bot on the Szeged master.
Balazs Kelemen
Comment 12
2011-08-05 09:09:21 PDT
(In reply to
comment #11
)
> Seems like it broke the Mac and the 64bit bot on the Szeged master.
After another build it's more likely that it is flakiness. One test is still failing on the Mac bot but it's not one of the 2 failure that was appearing first time after this change.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug