Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
524593003:
Resolve direction correctly when dir attribute is not in a defined state
|
Created:
6 years, 3 months ago by Sunil Ratnu Modified:
6 years ago CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionResolve direction correctly when dir attribute is not in a defined state
The current spec for directionality:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#the-directionality says that:
"If the element has a parent element and the dir attribute is not in a defined
state (i.e. it is not present or has an invalid value), The directionality of
the element is the same as the element's parent element's directionality."
BUG=411865
Patch Set 1 #Patch Set 2 : Added code for resolving direction #
Total comments: 2
Patch Set 3 : Splitting CL into 2 #
Total comments: 11
Patch Set 4 : Incorporating review comments #
Total comments: 4
Patch Set 5 : Let style resolver take care of direction #
Total comments: 9
Patch Set 6 : #Patch Set 7 : Fix failing test #
Total comments: 5
Messages
Total messages: 38 (5 generated)
sunil.ratnu@samsung.com changed reviewers: + tkent@chromium.org
Please review this. Thanks!
Need tests. What about other browser behavior?
On 2014/09/02 09:53:34, tkent wrote: > Need tests. > > What about other browser behavior? Hi tkent, I have added the test cases as well as linked the bug mentioning the behaviour in different browsers. Please review this. Thanks!
Let's split the CL to - a change for HTMLElement - a change for input/textarea. The rule [1] affects all of HTML elements. Let's land a change for it first to see if it won't make bad regressions. Changing multiple behaviors at once is very risky. [1] "If the element has a parent element and the dir attribute is not in a defined state (i.e. it is not present or has an invalid value)" https://codereview.chromium.org/524593003/diff/20001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/20001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:195: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, directionForFormData()); |ForFormData| is an inappropriate name. FormData is unrelated here.
Thanks tkent for the quick review. I will soon upload new patches with your suggested changes.
Hi tkent, I've made the suggested changes. Please have a look. Thanks! https://codereview.chromium.org/524593003/diff/20001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/20001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:195: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, directionForFormData()); On 2014/09/09 01:42:28, tkent wrote: > |ForFormData| is an inappropriate name. FormData is unrelated here. Done.
https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html (right): https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html:1: <html> The CL is independent from forms. Probably this test should be in fast/html/. https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (!isValidDirAttribute(value)) { Looks incorrect. If the value is "auto", we need to call directionalityIfhasDirAutoAttribute(). Probably this code block should be just addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, direction() == RTL ? "rtl" : "ltr"); https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:687: String HTMLElement::direction() const This function should return TextDirection because it returns either of "rtl" or "ltr". Also, please add FIXME about spec compliance.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute-expected.html (right): https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute-expected.html:1: <html> Missing doctype. https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html (right): https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html:1: <html> ditto. https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (!isValidDirAttribute(value)) { On 2014/09/09 at 06:37:00, tkent (overloaded) wrote: > Looks incorrect. > If the value is "auto", we need to call directionalityIfhasDirAutoAttribute(). > Doesn't that happen inside the direction() method?
Thanks tkent and esprehn for the review. I've made the suggested changes, please take a look. Thanks! https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute-expected.html (right): https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute-expected.html:1: <html> On 2014/09/09 10:20:13, esprehn wrote: > Missing doctype. Done. https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... File LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html (right): https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html:1: <html> On 2014/09/09 06:37:00, tkent (overloaded) wrote: > The CL is independent from forms. Probably this test should be in fast/html/. Done. https://codereview.chromium.org/524593003/diff/40001/LayoutTests/fast/forms/d... LayoutTests/fast/forms/direction-in-case-of-invalid-dir-attribute.html:1: <html> On 2014/09/09 10:20:13, esprehn wrote: > ditto. Done. https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (!isValidDirAttribute(value)) { This check is for the case when the dir attribute is missing or has an invalid value in which case we go on to search for the directionality of the parent. Earlier, what we used to do is that whenever we encountered an invalid dir attribute, we used to set the value to "ltr" directionalityIfhasDirAutoAttribute() gets called inside the direction() method. https://codereview.chromium.org/524593003/diff/40001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:687: String HTMLElement::direction() const On 2014/09/09 06:37:00, tkent (overloaded) wrote: > This function should return TextDirection because it returns either of "rtl" or > "ltr". > > Also, please add FIXME about spec compliance. Done.
https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:192: const AtomicString& dirValue = determineDirection() == RTL ? "rtl" : "ltr"; I realized that we didn't need to compute direction here. We should do nothing if dir= value is invalid, and the style resolver will work well for parent lookup. https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:195: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value); This can set "auto" to "direction" CSS property. It's wrong.
Thanks tkent for the review. I've made the suggested changes. Please have a look. Thanks! https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:192: const AtomicString& dirValue = determineDirection() == RTL ? "rtl" : "ltr"; On 2014/09/10 01:08:52, tkent (overloaded) wrote: > I realized that we didn't need to compute direction here. We should do nothing > if dir= value is invalid, and the style resolver will work well for parent > lookup. Done. https://codereview.chromium.org/524593003/diff/60001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:195: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value); We don't have this code part now since whenever we encounter an invalid dir attribute, we do nothing and the style resolver will take care for the parent lookup.
Gentle ping. Please review the latest changes. Thanks!
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (isValidDirAttribute(value)) If |value| is "auto", this code sets it to direction property. Does it conform to the standard?
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (isValidDirAttribute(value)) On 2014/09/12 06:33:55, tkent (overloaded) wrote: > If |value| is "auto", this code sets it to direction property. Does it conform > to the standard? I don't think the flow will come here for value 'auto' because it is in the else block. Do tell me if i am missing something. Thanks!
Hi tkent, can you please have a look?
On 2014/09/18 03:00:30, Sunil Ratnu wrote: > Hi tkent, can you please have a look? Gentle ping!
On 2014/09/23 19:50:50, Sunil Ratnu wrote: > On 2014/09/18 03:00:30, Sunil Ratnu wrote: > > Hi tkent, can you please have a look? > > Gentle ping! Please have a look. Thanks!
Hi tkent, Could you please review this. Thanks! Sunil
sunil.ratnu@samsung.com changed reviewers: + keishi@chromium.org
On 2014/10/06 10:33:24, Sunil Ratnu wrote: > Hi tkent, > > Could you please review this. > > Thanks! > Sunil Hi keishi, tkent seems to be OOO. Could you please review this CL? Thanks! Sunil
On 2014/10/09 06:59:54, Sunil Ratnu wrote: > On 2014/10/06 10:33:24, Sunil Ratnu wrote: > > Hi tkent, > > > > Could you please review this. > > > > Thanks! > > Sunil > > Hi keishi, > > tkent seems to be OOO. Could you please review this CL? > > Thanks! > Sunil Hi tkent, Could you please review this CL. Its been pending for a long time now. Thanks! Sunil
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (isValidDirAttribute(value)) On 2014/09/12 07:16:12, Sunil Ratnu wrote: > On 2014/09/12 06:33:55, tkent (overloaded) wrote: > > If |value| is "auto", this code sets it to direction property. Does it > conform > > to the standard? > > I don't think the flow will come here for value 'auto' because it is in the else > block. Do tell me if i am missing something. Thanks! > Why is it in the else block? Did you look at the isValidDirAttribute code?
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (isValidDirAttribute(value)) On 2014/11/07 01:01:53, tkent wrote: > On 2014/09/12 07:16:12, Sunil Ratnu wrote: > > On 2014/09/12 06:33:55, tkent (overloaded) wrote: > > > If |value| is "auto", this code sets it to direction property. Does it > > conform > > > to the standard? > > > > I don't think the flow will come here for value 'auto' because it is in the > else > > block. Do tell me if i am missing something. Thanks! > > > > Why is it in the else block? Did you look at the isValidDirAttribute code? Yes, I did take a look at the isValidDirAttribute and it qualifies "ltr", "rtl", "auto" as valid dir attributes. The css direction property specifies the text direction/writing direction and it can take values of "ltr", "rtl", "initial" and "inherit", so we do not want to set an "auto" value (i.e. and invalid value for css direction property) to the css direction property, hence, it is in the else block.
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:191: if (isValidDirAttribute(value)) On 2014/09/12 07:16:12, Sunil Ratnu wrote: > On 2014/09/12 06:33:55, tkent (overloaded) wrote: > > If |value| is "auto", this code sets it to direction property. Does it > conform > > to the standard? > > I don't think the flow will come here for value 'auto' because it is in the else > block. Do tell me if i am missing something. Thanks! > Ah, you're right. We check "auto" at line 188. Sorry for the confusion.
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.cpp:684: // FIXME: Implement directionality for input type='tel' when it is supported type=tel is supported by Google Chrome, IE, Firefox, and Safari. https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.h:84: TextDirection determineDirection() const; Only form controls use this function. So we have no reason to rename it and move it to HTMLElement.
Hi tkent, Please have a look. Thanks! Sunil https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.h:84: TextDirection determineDirection() const; On 2014/11/19 03:38:34, tkent wrote: > Only form controls use this function. So we have no reason to rename it and > move it to HTMLElement. The spec says that the directionality of an element (any element, not just an HTML element) is either 'ltr' or 'rtl', and is determined as per the first appropriate set of steps from the following list. If the element is an input element whose type attribute is in the Telephone state, and the dir attribute is not in a defined state, The directionality of the element is 'ltr'. So if we are planning to have this check, then we might need to move it to HTMLElement. We can do this in a separate CL, WDYT?
The CQ bit was checked by tkent@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/524593003/100001
https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... File Source/core/html/HTMLElement.h (right): https://codereview.chromium.org/524593003/diff/80001/Source/core/html/HTMLEle... Source/core/html/HTMLElement.h:84: TextDirection determineDirection() const; On 2014/11/19 05:33:25, Sunil Ratnu wrote: > On 2014/11/19 03:38:34, tkent wrote: > > Only form controls use this function. So we have no reason to rename it and > > move it to HTMLElement. > > The spec says that the directionality of an element (any element, not just an > HTML element) is either 'ltr' or 'rtl', and is determined as per the first > appropriate set of steps from the following list. > > If the element is an input element whose type attribute is in the Telephone > state, and the dir attribute is not in a defined state, The directionality of > the element is 'ltr'. So if we are planning to have this check, then we might > need to move it to HTMLElement. We can do this in a separate CL, WDYT? We should add |virtual AtomicString effectiveDirAttributeValue(const AtomicString& value)| to HTMLElement, HTMLElement::collectStyleForPresentationAttribute() calls it, and HTMLInputElement overrides it so that it returns "ltr" for type=tel.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)
Hi tkent, The previous change was breaking fast/dom/Attr/direction-attribute-set-and-cleared.html, so I've made a few changes (i.e. followed the previous approach). Please have a look and do let me know your opinion. Thanks! Sunil https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:217: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "ltr"); Hi tkent, We need to keep this else check here because removing it breaks fast/dom/Attr/direction-attribute-set-and-cleared.html test. The parent lookup fails here and keeps the direction "rtl" even after clearing it to "" as mentioned in the test. So, we need to make sure that we consider parent element's directionality and resolve accordingly. The mentioned test fails no more now. https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:722: TextDirection HTMLElement::determineDirection() const Instead of moving it here, we can also define a new function effectiveDirAttributeValue (as suggested by you in case of input type='tel') but that also has to use a lot of same code here. Hence, I moved it from HTMLTextFormControlElement.cpp in order to reduce the duplicated code.
https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:217: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "ltr"); On 2014/11/21 12:21:08, Sunil Ratnu wrote: > Hi tkent, We need to keep this else check here because removing it breaks > fast/dom/Attr/direction-attribute-set-and-cleared.html test. The parent lookup > fails here and keeps the direction "rtl" even after clearing it to "" as > mentioned in the test. So, we need to make sure that we consider parent > element's directionality and resolve accordingly. The mentioned test fails no > more now. I don't think parent lookup is a right solution. How about style->removeProperty(CSSPropertyDirection); ?
https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:217: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "ltr"); On 2014/11/25 01:27:24, tkent wrote: > On 2014/11/21 12:21:08, Sunil Ratnu wrote: > > Hi tkent, We need to keep this else check here because removing it breaks > > fast/dom/Attr/direction-attribute-set-and-cleared.html test. The parent lookup > > fails here and keeps the direction "rtl" even after clearing it to "" as > > mentioned in the test. So, we need to make sure that we consider parent > > element's directionality and resolve accordingly. The mentioned test fails no > > more now. > > I don't think parent lookup is a right solution. > How about > style->removeProperty(CSSPropertyDirection); > ? The above approach does not work here. The above mentioned test still fails. Since this is the only place where we are checking for invalid dirAttr or clearing of dirAttr and finally adding the direction to CSSPropertyDirection, we need parent lookup so sa to resolve the direction and that seems reasonable.
https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... File Source/core/html/HTMLElement.cpp (left): https://codereview.chromium.org/524593003/diff/120001/Source/core/html/HTMLEl... Source/core/html/HTMLElement.cpp:217: addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, "ltr"); On 2014/11/25 05:43:25, Sunil Ratnu wrote: > On 2014/11/25 01:27:24, tkent wrote: > > On 2014/11/21 12:21:08, Sunil Ratnu wrote: > > > Hi tkent, We need to keep this else check here because removing it breaks > > > fast/dom/Attr/direction-attribute-set-and-cleared.html test. The parent > lookup > > > fails here and keeps the direction "rtl" even after clearing it to "" as > > > mentioned in the test. So, we need to make sure that we consider parent > > > element's directionality and resolve accordingly. The mentioned test fails > no > > > more now. > > > > I don't think parent lookup is a right solution. > > How about > > style->removeProperty(CSSPropertyDirection); > > ? > > The above approach does not work here. The above mentioned test still fails. > Since this is the only place where we are checking for invalid dirAttr or > clearing of dirAttr and finally adding the direction to CSSPropertyDirection, we > need parent lookup so sa to resolve the direction and that seems reasonable. Please investigate why it didn't work, and find another solution without parent lookup. Adding parent lookup is not acceptable. Style resolution already contains parent lookup, and adding DOM parent lookup will introduce inconsistency and confusion. |
