Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
389343002:
Custom validation message to take into account text direction (Closed)
|
Created:
6 years, 5 months ago by Habib Virji Modified:
6 years, 4 months ago CC:
abarth-chromium, blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, jamesr Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionCustom validation message to take into account text direction
Custom validation message, to use for main message first character direction. For subMessage, if direction is auto, then first character direction is applied, else RTL/LTR values are applied.
* HTMLFormControlElement.cpp:
Helper function for custom message, for the main message determineDirectionality is used. Sub Message uses element direction, if element direction is not present, then style direction is used.
* ValidationMessageClientImpl.cpp
Change of direction to web direction.
* ValidationMessageClient.h
ValidationMessageClientImpl.h
Pass direction and sub message information.
* WebViewClient.h
Header file that will be used by chrome side and includes extra direction for subMsg. Old function is kept as placeholder till landing patch.
BUG=91638
R=tkent, leviw
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180069
Patch Set 1 #
Total comments: 4
Patch Set 2 : Compilation and Levi's firstStrong changes #
Total comments: 6
Patch Set 3 : Introduced enum type for message type, helper function to detect direction and placeholder function… #Patch Set 4 : Fix failing test #
Total comments: 5
Patch Set 5 : Moved code from web to core #
Total comments: 10
Patch Set 6 : Addressed code review comments #
Total comments: 8
Patch Set 7 : Corrected RTL text in the test #Patch Set 8 : Added back direction check and layout test updated to use result from web_test_proxy. #Patch Set 9 : Added unit test file and made findCustomValidationMessageTextDirection as a member function #Patch Set 10 : Updated to latest master changes #
Total comments: 1
Patch Set 11 : Removed |dir| value read. Updated tests to use setDirection. #
Total comments: 12
Patch Set 12 : Code review comments #
Total comments: 14
Patch Set 13 : Corrected expected format in the test #Patch Set 14 : Corrected addition in core.gypi #
Messages
Total messages: 41 (0 generated)
Please note, I have not added test as was not able to find that allow checking these validation message. Please suggest test. Chrome side changes: https://codereview.chromium.org/387353002/
On 2014/07/14 14:47:52, Habib Virji wrote: > Please note, I have not added test as was not able to find that allow checking > these validation message. Please suggest test. > > Chrome side changes: https://codereview.chromium.org/387353002/ Updating to latest master.
This isn't right yet. https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... Source/web/ValidationMessageClientImpl.cpp:69: static WebTextDirection firstStrongDirection(String message) Shouldn't you be iterating over the string until you find the first character with a strong direction? We have other code that runs the first part of the UBA to find the directionality of a string. Look for "determineDirectionality" https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... Source/web/ValidationMessageClientImpl.cpp:73: return charDirection == WTF::Unicode::LeftToRight ? WebTextDirectionLeftToRight: WebTextDirectionRightToLeft; This seems wrong. If the *first character* is something like a quotation mark which isn't strong LTR, we'll use RTL, which is not our normal fallback.
Updated to latest master (ValidationMessage was deleted from forms, so some changes) and updated to leviw suggestion for finding firstStrong. https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... Source/web/ValidationMessageClientImpl.cpp:69: static WebTextDirection firstStrongDirection(String message) On 2014/07/14 19:13:50, leviw wrote: > Shouldn't you be iterating over the string until you find the first character > with a strong direction? > > We have other code that runs the first part of the UBA to find the > directionality of a string. Look for "determineDirectionality" Agreed, updated code to use determineDirectionality and seems to work fine. https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... Source/web/ValidationMessageClientImpl.cpp:73: return charDirection == WTF::Unicode::LeftToRight ? WebTextDirectionLeftToRight: WebTextDirectionRightToLeft; On 2014/07/14 19:13:50, leviw wrote: > This seems wrong. If the *first character* is something like a quotation mark > which isn't strong LTR, we'll use RTL, which is not our normal fallback. Done.
On 2014/07/15 16:33:31, Habib Virji wrote: > Updated to latest master (ValidationMessage was deleted from forms, so some > changes) and updated to leviw suggestion for finding firstStrong. > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > File Source/web/ValidationMessageClientImpl.cpp (right): > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > Source/web/ValidationMessageClientImpl.cpp:69: static WebTextDirection > firstStrongDirection(String message) > On 2014/07/14 19:13:50, leviw wrote: > > Shouldn't you be iterating over the string until you find the first character > > with a strong direction? > > > > We have other code that runs the first part of the UBA to find the > > directionality of a string. Look for "determineDirectionality" > > Agreed, updated code to use determineDirectionality and seems to work fine. > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > Source/web/ValidationMessageClientImpl.cpp:73: return charDirection == > WTF::Unicode::LeftToRight ? WebTextDirectionLeftToRight: > WebTextDirectionRightToLeft; > On 2014/07/14 19:13:50, leviw wrote: > > This seems wrong. If the *first character* is something like a quotation mark > > which isn't strong LTR, we'll use RTL, which is not our normal fallback. > > Done. @tkent: Compilation will fail since chromium side are needed for full compilation. Should I use dummy functions, to make compilation fix.
On 2014/07/15 16:43:55, Habib Virji wrote: > On 2014/07/15 16:33:31, Habib Virji wrote: > > Updated to latest master (ValidationMessage was deleted from forms, so some > > changes) and updated to leviw suggestion for finding firstStrong. > > > > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > > File Source/web/ValidationMessageClientImpl.cpp (right): > > > > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > > Source/web/ValidationMessageClientImpl.cpp:69: static WebTextDirection > > firstStrongDirection(String message) > > On 2014/07/14 19:13:50, leviw wrote: > > > Shouldn't you be iterating over the string until you find the first > character > > > with a strong direction? > > > > > > We have other code that runs the first part of the UBA to find the > > > directionality of a string. Look for "determineDirectionality" > > > > Agreed, updated code to use determineDirectionality and seems to work fine. > > > > > https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessage... > > Source/web/ValidationMessageClientImpl.cpp:73: return charDirection == > > WTF::Unicode::LeftToRight ? WebTextDirectionLeftToRight: > > WebTextDirectionRightToLeft; > > On 2014/07/14 19:13:50, leviw wrote: > > > This seems wrong. If the *first character* is something like a quotation > mark > > > which isn't strong LTR, we'll use RTL, which is not our normal fallback. > > > > Done. > > @tkent: Compilation will fail since chromium side are needed for full > compilation. Should I use dummy functions, to make compilation fix. Chrome side changes are available here: https://codereview.chromium.org/391923006
https://codereview.chromium.org/389343002/diff/20001/Source/core/page/Validat... File Source/core/page/ValidationMessageClient.h (right): https://codereview.chromium.org/389343002/diff/20001/Source/core/page/Validat... Source/core/page/ValidationMessageClient.h:44: virtual void showValidationMessage(const Element& anchor, const String& message, bool customError) = 0; boolean parameters make me sad. Can we use something else like an enum? https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:105: } Can you put all this code into a helper function? https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:107: m_finishTime = monotonicallyIncreasingTime() + std::max(minimumSecondToShowValidationMessage, (message.length() + subMsg.length()) * secondPerCharacter); It seems like this code doesn't need to be duplicated.
I am not sure how to land changes with chromium side. I have left old function prototype and calling to match with current chrome. (Not sure if its right approach, but might make things work) https://codereview.chromium.org/389343002/diff/20001/Source/core/page/Validat... File Source/core/page/ValidationMessageClient.h (right): https://codereview.chromium.org/389343002/diff/20001/Source/core/page/Validat... Source/core/page/ValidationMessageClient.h:44: virtual void showValidationMessage(const Element& anchor, const String& message, bool customError) = 0; On 2014/07/16 04:26:22, leviw wrote: > boolean parameters make me sad. Can we use something else like an enum? I have introduced enum type and removed boolean now. https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:105: } On 2014/07/16 04:26:22, leviw wrote: > Can you put all this code into a helper function? Done. https://codereview.chromium.org/389343002/diff/20001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:107: m_finishTime = monotonicallyIncreasingTime() + std::max(minimumSecondToShowValidationMessage, (message.length() + subMsg.length()) * secondPerCharacter); On 2014/07/16 04:26:22, leviw wrote: > It seems like this code doesn't need to be duplicated. It had subMsg.length which differed, changed to avoid duplication.
On 2014/07/16 13:32:02, Habib Virji wrote: > I am not sure how to land changes with chromium side. I have left old function > prototype and calling to match with current chrome. (Not sure if its right > approach, but might make things work) Yeah, the approach will work.
We prefer that web/ implementation is small. IMO, we should determine text direction in core/, and add direction arguments (and sub-message string argument) to ValidationMessageClient::showValidationMessage. https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:110: messageDir = m_currentAnchor->locale().isRTL() ? WebTextDirectionRightToLeft: WebTextDirectionLeftToRight; Looking at the element locale is wrong at this moment. Canned messages are based on browser UI locale, and an element locale depends on a web page. Why don't you use determineDirectionality() for canned messages too? https://codereview.chromium.org/389343002/diff/60001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/389343002/diff/60001/public/web/WebViewClient... public/web/WebViewClient.h:166: virtual void showValidationMessage(const WebRect& anchorInRootView, const WebString& mainText, WebTextDirection mainTextDir, const WebString& supplementalText, WebTextDirection hint) { } |hint| should be renamed to |supplementalTextDir|.
Moved code from web to core. Other thing I am not clear about is how to treat canned message, this patch does not have a canned message check as I was not able to find a way to get browser UI locale information. https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:110: messageDir = m_currentAnchor->locale().isRTL() ? WebTextDirectionRightToLeft: WebTextDirectionLeftToRight; On 2014/07/18 13:57:49, tkent wrote: > Looking at the element locale is wrong at this moment. Canned messages are > based on browser UI locale, and an element locale depends on a web page. > Why don't you use determineDirectionality() for canned messages too? Okay, I was more looking for browser UI locale. As per Haron's comment: https://code.google.com/p/chromium/issues/detail?id=91638#c26, he said canned message to follow brower locale. Is there way of accessing this information in core? https://codereview.chromium.org/389343002/diff/60001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/389343002/diff/60001/public/web/WebViewClient... public/web/WebViewClient.h:166: virtual void showValidationMessage(const WebRect& anchorInRootView, const WebString& mainText, WebTextDirection mainTextDir, const WebString& supplementalText, WebTextDirection hint) { } On 2014/07/18 13:57:49, tkent wrote: > |hint| should be renamed to |supplementalTextDir|. Done.
This CL should have unit test. https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMes... Source/web/ValidationMessageClientImpl.cpp:110: messageDir = m_currentAnchor->locale().isRTL() ? WebTextDirectionRightToLeft: WebTextDirectionLeftToRight; On 2014/07/18 15:37:45, Habib Virji wrote: > On 2014/07/18 13:57:49, tkent wrote: > > Looking at the element locale is wrong at this moment. Canned messages are > > based on browser UI locale, and an element locale depends on a web page. > > Why don't you use determineDirectionality() for canned messages too? > > Okay, I was more looking for browser UI locale. As per Haron's comment: > https://code.google.com/p/chromium/issues/detail?id=91638#c26, he said canned > message to follow brower locale. Is there way of accessing this information in > core? There is. But using determineDirectionality for canned messages should have the same result as obtaining direction of the browser locale. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:409: static void findCustomMessageTextDirection(const Element* anchor, const String& message, TextDirection& messageDir, String& subMessage, TextDirection& subMessageDir) |anchor| should be |const Element&|, or this function should be a member function of HTMLFormControlElement. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:412: subMessage = anchor->fastGetAttribute(HTMLNames::titleAttr); You can omit HTMLNames::. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:416: AtomicString dir = anchor->fastGetAttribute(HTMLNames::dirAttr); Don't refer to |dir| attribute of the element to determine element's direction. It's not a correct way. e.g. <div dir=rtl> <input title="...."> </div> The input is in RTL. We should refer to renderer()->style()->direction() first. https://codereview.chromium.org/389343002/diff/100001/Source/core/page/Valida... File Source/core/page/ValidationMessageClient.h (right): https://codereview.chromium.org/389343002/diff/100001/Source/core/page/Valida... Source/core/page/ValidationMessageClient.h:38: This change is unnecessary. https://codereview.chromium.org/389343002/diff/100001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/100001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.cpp:86: unsigned length = message.length() + (!subMessage.isNull() ? subMessage.length() : 0); Checking isNull is unnecessary. subMessage.length() returns 0 even if isNull. Also, I don't understand why you added |length| local variable.
@tkent: thanks for reviewing. Have worked on all suggested changes. Regarding test, i am bit stuck. Problem I am facing is setCustomValidation allow setting custom message, but how to check validationMessage direction. In test I was trying to set title and custom validation message, then determine direction, i failed to determine direction as did not find a way to get a direction information. Regarding subMsg direction, using style direction by default and only changing if direction is auto. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:409: static void findCustomMessageTextDirection(const Element* anchor, const String& message, TextDirection& messageDir, String& subMessage, TextDirection& subMessageDir) On 2014/07/22 01:29:56, tkent wrote: > |anchor| should be |const Element&|, or this function should be a member > function of HTMLFormControlElement. > Done. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:412: subMessage = anchor->fastGetAttribute(HTMLNames::titleAttr); On 2014/07/22 01:29:56, tkent wrote: > You can omit HTMLNames::. Done. https://codereview.chromium.org/389343002/diff/100001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:416: AtomicString dir = anchor->fastGetAttribute(HTMLNames::dirAttr); On 2014/07/22 01:29:56, tkent wrote: > Don't refer to |dir| attribute of the element to determine element's direction. > It's not a correct way. e.g. > <div dir=rtl> > <input title="...."> > </div> > The input is in RTL. > > We should refer to renderer()->style()->direction() first. Done. https://codereview.chromium.org/389343002/diff/100001/Source/core/page/Valida... File Source/core/page/ValidationMessageClient.h (right): https://codereview.chromium.org/389343002/diff/100001/Source/core/page/Valida... Source/core/page/ValidationMessageClient.h:38: On 2014/07/22 01:29:56, tkent wrote: > This change is unnecessary. Done. https://codereview.chromium.org/389343002/diff/100001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/100001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.cpp:86: unsigned length = message.length() + (!subMessage.isNull() ? subMessage.length() : 0); On 2014/07/22 01:29:56, tkent wrote: > Checking isNull is unnecessary. subMessage.length() returns 0 even if isNull. > Also, I don't understand why you added |length| local variable. Done.
> Regarding test, i am bit stuck. Problem I am facing is setCustomValidation allow > setting custom message, but how to check validationMessage direction. In test I > was trying to set title and custom validation message, then determine direction, > i failed to determine direction as did not find a way to get a direction information. I don't understand what is your problem. Please post a patch with your incomplete unit test. https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:415: // Sub Msg takes direction of the element direction Msg -> message https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:417: AtomicString dir = anchor.fastGetAttribute(dirAttr); Line 417-419 has no effect. It is unnecessary. https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:437: TextDirection messageDir, subMessageDir = LTR; We don't declare multiple variables at once. https://codereview.chromium.org/389343002/diff/120001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.h (right): https://codereview.chromium.org/389343002/diff/120001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.h:56: virtual void showValidationMessage(const WebCore::Element& anchor, const String& message, WebCore::TextDirection messageDir, const String& subMessage, WebCore::TextDirection subMessageDir) OVERRIDE; Please use blink:: namespace.
Added test. The result in the expected file matches if steps are run manually in chrome. FAIL test.validationMessage should be .Main Message ﻰﺑﺮﻋ. Was ﻰﺑﺮﻋ Main Message.. FAIL test.title should be ﻰﺑﺮﻋ Sub Message.. Was ىبرع Sub Message.. Setting element direction as RTL, results in sub message using direction RTL FAIL test.validationMessage should be .Main Message ﻰﺑﺮﻋ. Was ﻰﺑﺮﻋ Main Message.. FAIL test.title should be .Sub Message ﻰﺑﺮﻋ. Was ىبرع Sub Message.. https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:415: // Sub Msg takes direction of the element direction On 2014/07/22 23:56:48, tkent wrote: > Msg -> message Done. https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:417: AtomicString dir = anchor.fastGetAttribute(dirAttr); On 2014/07/22 23:56:49, tkent wrote: > Line 417-419 has no effect. It is unnecessary. Done. Removed but it did work correctly if <input dir=auto> was present. https://codereview.chromium.org/389343002/diff/120001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:437: TextDirection messageDir, subMessageDir = LTR; On 2014/07/22 23:56:49, tkent wrote: > We don't declare multiple variables at once. Done. https://codereview.chromium.org/389343002/diff/120001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.h (right): https://codereview.chromium.org/389343002/diff/120001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.h:56: virtual void showValidationMessage(const WebCore::Element& anchor, const String& message, WebCore::TextDirection messageDir, const String& subMessage, WebCore::TextDirection subMessageDir) OVERRIDE; On 2014/07/22 23:56:49, tkent wrote: > Please use blink:: namespace. Done.
This is the result of the test if the above test is run. (The result in the expected file matches if steps are run manually in chrome.) FAIL test.validationMessage should be .Main Message عربى. Was عربى Main Message.. PASS test.title is "عربى Sub Message." Setting element direction as RTL, results in sub message using direction RTL FAIL test.validationMessage should be .Main Message عربى. Was عربى Main Message.. FAIL test.title should be .Sub Message عربى. Was عربى Sub Message.. Probable reason is the text direction is found on the blink side but the message is not reversed. Reversing of text happens on the chrome side. So these tests fail.
I wrote "This CL should have unit test." It doesn't mean a layout test. We can't test this with a layout test because this CL won't change any DOM properties.
Apologise, for missing to read unit test.
Did try to make unit test work in WebViewTest.cpp
1. Few things missing were missing validationMessage and title in
WebFormControlElement. Implemented to pass validaionMessage as WebString in
WebFormControlElement.
This is what I tried:
TEST_F(WebViewTest, ValidationMessageTextDirection)
{
URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()),
WebString::fromUTF8("customValidationMessageDirection.html"));
WebView* webView = m_webViewHelper.initializeAndLoad(m_baseURL +
"customValidationMessageDirection.html", true);
WebElement webElement =
webView->mainFrame()->document().getElementById("text");
WebInputElement* element = toWebInputElement(&webElement);
base::string16 wrapped_main_text = element->validationMessage();
EXPECT_EQ(element.validationMessage(), "ﻉﺮﺑﻯ Sub Message.");
}
--> But the issue still persists that validationMessage is never changed, it
remains same. The change on blink only checks direction and on chrome side, the
string is appended with RTL and LTR information.
The possible test on blink side, is check direction and probably that's
irrelevant.
2. So that's why I updated web_test_proxy.cc, as some test prints out the value
returned (ValidationClientImpl). On chrome side only string is appended with LRE
and RLE information, string is not reversed, and this information can be availed
from this function. (UI seems to have some mechanism where these appended
information can convert the string, how this conversion happens I could not
fathom).
So, on the blink side, I have still tried using layout test not to check DOM
properties but to check what value ShowValidationMessage returns.
Other changes are:
1. Re-introducing of .dir functionality. Reason is in above script when I
changed direction using text.style.direction, it does not result in
renderer()->style()->direction returning back RTL. But it does detect if dir()
is used to find direction. Also if <input dir=auto> is present, it works
correctly even if <div dir=rtl> <input dir=auto title="message"/> </div>,
direction ltr is returned for the message.
2. Change method pass parameter to make it simplified.
The core of the CL is findCustomValidationMessageTextDirection(). You should add HTMLFormControlElementTest.cpp and add a test to it. Please do not add a layout test and |dir| check. They don't make sense.
On 2014/07/28 01:30:15, tkent wrote: > The core of the CL is findCustomValidationMessageTextDirection(). You should > add HTMLFormControlElementTest.cpp and add a test to it. add a test -> add a test for findCustomValidationMessageTextDirection()
On 2014/07/28 01:31:03, tkent wrote: > On 2014/07/28 01:30:15, tkent wrote: > > The core of the CL is findCustomValidationMessageTextDirection(). You should > > add HTMLFormControlElementTest.cpp and add a test to it. > > add a test -> add a test for findCustomValidationMessageTextDirection() Thanks tkent, have added now the unit test suggested.
On 2014/07/28 17:09:40, Habib Virji wrote: > On 2014/07/28 01:31:03, tkent wrote: > > On 2014/07/28 01:30:15, tkent wrote: > > > The core of the CL is findCustomValidationMessageTextDirection(). You > should > > > add HTMLFormControlElementTest.cpp and add a test to it. > > > > add a test -> add a test for findCustomValidationMessageTextDirection() > > Thanks tkent, have added now the unit test suggested. Also made findCustomValidationMessageTextDirection as a member function, to ease in calling in unit test.
On 2014/07/28 17:13:38, Habib Virji wrote: > On 2014/07/28 17:09:40, Habib Virji wrote: > > On 2014/07/28 01:31:03, tkent wrote: > > > On 2014/07/28 01:30:15, tkent wrote: > > > > The core of the CL is findCustomValidationMessageTextDirection(). You > > should > > > > add HTMLFormControlElementTest.cpp and add a test to it. > > > > > > add a test -> add a test for findCustomValidationMessageTextDirection() > > > > Thanks tkent, have added now the unit test suggested. > > Also made findCustomValidationMessageTextDirection as a member function, to ease > in calling in unit test. @tkent, are the changes ok
https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElement.cpp:416: AtomicString dir = fastGetAttribute(dirAttr); Why do you refer to |dir| attribute though we already have direction from renderer()->style()? As I wrote repeatedly, referring to |dir| attribute won't make a correct result. Do you understand what the direction of <input dir="rtl" style="direction:ltr;">?
On 2014/08/08 01:41:14, tkent wrote: > https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFo... > File Source/core/html/HTMLFormControlElement.cpp (right): > > https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFo... > Source/core/html/HTMLFormControlElement.cpp:416: AtomicString dir = > fastGetAttribute(dirAttr); > Why do you refer to |dir| attribute though we already have direction from > renderer()->style()? > > As I wrote repeatedly, referring to |dir| attribute won't make a correct result. > Do you understand what the direction of <input dir="rtl" > style="direction:ltr;">? Your point is valid, reason I included was due to not able to detect when changing direction in JavaScript: https://codereview.chromium.org/389343002/#msg18 "Reason is if I change direction using text.style.direction, it does not result in renderer()->style()->direction returning back RTL. But it does detect if dir() is used to find direction. Also if <input dir=auto> is present, it works correctly even if <div dir=rtl> <input dir=auto title="message"/> </div>, direction ltr is returned for the message."
On 2014/08/08 08:30:16, Habib Virji wrote: > Your point is valid, reason I included was due to not able to detect when > changing direction in JavaScript: When JavaScript code updates |dir| attribute, RenderStyle::direction is re-computed including |dir| value.
On 2014/08/08 09:21:09, tkent wrote: > On 2014/08/08 08:30:16, Habib Virji wrote: > > Your point is valid, reason I included was due to not able to detect when > > changing direction in JavaScript: > > When JavaScript code updates |dir| attribute, RenderStyle::direction is > re-computed including |dir| value. Thanks, uploaded new patch with suggested changes.
https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:14: using namespace blink; We may simply use blink namespace like: namespace blink { ... } instead of using namespace blink; namespace { .. } https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:39: m_document->documentElement()->setInnerHTML("<body><input required id=input></body>", ASSERT_NO_EXCEPTION); Please move setInnerHTML call to the TEST_F. HTMLFormControlElementTest can be used for other tests. https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:45: SetUp(); Do not call SetUp(). It is automatically called. https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:48: input->setCustomValidity(String::fromUTF8("عربى")); Please do not write non-ASCII characters in source code. fromUTF8("\xD8\xB9\xD8\xB1\xD8\xA8\xD9\x89") https://codereview.chromium.org/389343002/diff/240001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.h (right): https://codereview.chromium.org/389343002/diff/240001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.h:56: virtual void showValidationMessage(const blink::Element& anchor, const String& message, blink::TextDirection messageDir, const String& subMessage, blink::TextDirection subMessageDir) OVERRIDE; Please remove blink:: prefixes. https://codereview.chromium.org/389343002/diff/240001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/389343002/diff/240001/public/web/WebViewClien... public/web/WebViewClient.h:167: virtual void showValidationMessage(const WebRect& anchorInRootView, const WebString& mainText, const WebString& supplementalText, WebTextDirection hint) { } Add a FIXME comment that we'll remove this version.
Thanks, updated as per code review comments https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:14: using namespace blink; On 2014/08/11 01:45:48, tkent wrote: > We may simply use blink namespace like: > > namespace blink { > ... > } > > instead of > > using namespace blink; > namespace { > .. > } Done. https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:39: m_document->documentElement()->setInnerHTML("<body><input required id=input></body>", ASSERT_NO_EXCEPTION); On 2014/08/11 01:45:49, tkent wrote: > Please move setInnerHTML call to the TEST_F. > HTMLFormControlElementTest can be used for other tests. Done. https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:45: SetUp(); On 2014/08/11 01:45:48, tkent wrote: > Do not call SetUp(). It is automatically called. Done. https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:48: input->setCustomValidity(String::fromUTF8("عربى")); On 2014/08/11 01:45:48, tkent wrote: > Please do not write non-ASCII characters in source code. > > fromUTF8("\xD8\xB9\xD8\xB1\xD8\xA8\xD9\x89") Done. https://codereview.chromium.org/389343002/diff/240001/Source/web/ValidationMe... File Source/web/ValidationMessageClientImpl.h (right): https://codereview.chromium.org/389343002/diff/240001/Source/web/ValidationMe... Source/web/ValidationMessageClientImpl.h:56: virtual void showValidationMessage(const blink::Element& anchor, const String& message, blink::TextDirection messageDir, const String& subMessage, blink::TextDirection subMessageDir) OVERRIDE; On 2014/08/11 01:45:49, tkent wrote: > Please remove blink:: prefixes. Done. https://codereview.chromium.org/389343002/diff/240001/public/web/WebViewClient.h File public/web/WebViewClient.h (right): https://codereview.chromium.org/389343002/diff/240001/public/web/WebViewClien... public/web/WebViewClient.h:167: virtual void showValidationMessage(const WebRect& anchorInRootView, const WebString& mainText, const WebString& supplementalText, WebTextDirection hint) { } On 2014/08/11 01:45:49, tkent wrote: > Add a FIXME comment that we'll remove this version. Done.
lgtm https://codereview.chromium.org/389343002/diff/260001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/389343002/diff/260001/Source/core/core.gypi#n... Source/core/core.gypi:3450: 'html/HTMLFormControlElementTest.cpp', Sort alphabetically. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:54: EXPECT_EQ(messageDir, RTL); Please reverse the arguments. EXPECT_EQ(RTL, messageDir). The first argument should be an expected value, and the second argument should be an actual value. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:55: EXPECT_EQ(subMessageDir, LTR); Ditto. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:59: EXPECT_EQ(messageDir, RTL); Ditto. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:60: EXPECT_EQ(subMessageDir, RTL); Ditto. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:65: EXPECT_EQ(messageDir, LTR); Ditto. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:66: EXPECT_EQ(subMessageDir, RTL); Ditto.
Thanks for reviewing, updated as suggested. https://codereview.chromium.org/389343002/diff/260001/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/389343002/diff/260001/Source/core/core.gypi#n... Source/core/core.gypi:3450: 'html/HTMLFormControlElementTest.cpp', On 2014/08/12 00:19:58, tkent wrote: > Sort alphabetically. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:54: EXPECT_EQ(messageDir, RTL); On 2014/08/12 00:19:58, tkent wrote: > Please reverse the arguments. EXPECT_EQ(RTL, messageDir). > The first argument should be an expected value, and the second argument should > be an actual value. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:55: EXPECT_EQ(subMessageDir, LTR); On 2014/08/12 00:19:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:59: EXPECT_EQ(messageDir, RTL); On 2014/08/12 00:19:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:60: EXPECT_EQ(subMessageDir, RTL); On 2014/08/12 00:19:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:65: EXPECT_EQ(messageDir, LTR); On 2014/08/12 00:19:58, tkent wrote: > Ditto. Done. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFo... Source/core/html/HTMLFormControlElementTest.cpp:66: EXPECT_EQ(subMessageDir, RTL); On 2014/08/12 00:19:58, tkent wrote: > Ditto. Done.
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/280001
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/300001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22232)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22250)
The CQ bit was checked by habib.virji@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/300001
Message was sent while issue was closed.
Change committed as 180069 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
