Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(406)

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.

Description

Custom 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -10 lines) Patch
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.cpp View 1 2 3 4 5 6 7 8 9 10 3 chunks +15 lines, -1 line 0 comments Download
A Source/core/html/HTMLFormControlElementTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +69 lines, -0 lines 0 comments Download
M Source/core/page/ValidationMessageClient.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M Source/web/ValidationMessageClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M Source/web/ValidationMessageClientImpl.cpp View 1 2 3 4 5 2 chunks +7 lines, -7 lines 0 comments Download
M public/web/WebViewClient.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 41 (0 generated)
Habib Virji
Please note, I have not added test as was not able to find that allow ...
6 years, 5 months ago (2014-07-14 14:47:52 UTC) #1
Habib Virji
On 2014/07/14 14:47:52, Habib Virji wrote: > Please note, I have not added test as ...
6 years, 5 months ago (2014-07-14 16:03:42 UTC) #2
leviw_travelin_and_unemployed
This isn't right yet. https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessageClientImpl.cpp File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/1/Source/web/ValidationMessageClientImpl.cpp#newcode69 Source/web/ValidationMessageClientImpl.cpp:69: static WebTextDirection firstStrongDirection(String message) Shouldn't ...
6 years, 5 months ago (2014-07-14 19:13:50 UTC) #3
Habib Virji
Updated to latest master (ValidationMessage was deleted from forms, so some changes) and updated to ...
6 years, 5 months ago (2014-07-15 16:33:31 UTC) #4
Habib Virji
On 2014/07/15 16:33:31, Habib Virji wrote: > Updated to latest master (ValidationMessage was deleted from ...
6 years, 5 months ago (2014-07-15 16:43:55 UTC) #5
Habib Virji
On 2014/07/15 16:43:55, Habib Virji wrote: > On 2014/07/15 16:33:31, Habib Virji wrote: > > ...
6 years, 5 months ago (2014-07-15 16:50:17 UTC) #6
leviw_travelin_and_unemployed
https://codereview.chromium.org/389343002/diff/20001/Source/core/page/ValidationMessageClient.h File Source/core/page/ValidationMessageClient.h (right): https://codereview.chromium.org/389343002/diff/20001/Source/core/page/ValidationMessageClient.h#newcode44 Source/core/page/ValidationMessageClient.h:44: virtual void showValidationMessage(const Element& anchor, const String& message, bool ...
6 years, 5 months ago (2014-07-16 04:26:22 UTC) #7
Habib Virji
I am not sure how to land changes with chromium side. I have left old ...
6 years, 5 months ago (2014-07-16 13:32:02 UTC) #8
tkent
On 2014/07/16 13:32:02, Habib Virji wrote: > I am not sure how to land changes ...
6 years, 5 months ago (2014-07-18 13:48:52 UTC) #9
tkent
We prefer that web/ implementation is small. IMO, we should determine text direction in core/, ...
6 years, 5 months ago (2014-07-18 13:57:49 UTC) #10
Habib Virji
Moved code from web to core. Other thing I am not clear about is how ...
6 years, 5 months ago (2014-07-18 15:37:45 UTC) #11
tkent
This CL should have unit test. https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMessageClientImpl.cpp File Source/web/ValidationMessageClientImpl.cpp (right): https://codereview.chromium.org/389343002/diff/60001/Source/web/ValidationMessageClientImpl.cpp#newcode110 Source/web/ValidationMessageClientImpl.cpp:110: messageDir = m_currentAnchor->locale().isRTL() ...
6 years, 5 months ago (2014-07-22 01:29:57 UTC) #12
Habib Virji
@tkent: thanks for reviewing. Have worked on all suggested changes. Regarding test, i am bit ...
6 years, 5 months ago (2014-07-22 16:10:00 UTC) #13
tkent
> Regarding test, i am bit stuck. Problem I am facing is setCustomValidation allow > ...
6 years, 5 months ago (2014-07-22 23:56:49 UTC) #14
Habib Virji
Added test. The result in the expected file matches if steps are run manually in ...
6 years, 5 months ago (2014-07-23 13:32:04 UTC) #15
Habib Virji
This is the result of the test if the above test is run. (The result ...
6 years, 5 months ago (2014-07-23 13:53:02 UTC) #16
tkent
I wrote "This CL should have unit test." It doesn't mean a layout test. We ...
6 years, 5 months ago (2014-07-24 01:12:06 UTC) #17
Habib Virji
Apologise, for missing to read unit test. Did try to make unit test work in ...
6 years, 5 months ago (2014-07-25 15:42:34 UTC) #18
tkent
The core of the CL is findCustomValidationMessageTextDirection(). You should add HTMLFormControlElementTest.cpp and add a test ...
6 years, 4 months ago (2014-07-28 01:30:15 UTC) #19
tkent
On 2014/07/28 01:30:15, tkent wrote: > The core of the CL is findCustomValidationMessageTextDirection(). You should ...
6 years, 4 months ago (2014-07-28 01:31:03 UTC) #20
Habib Virji
On 2014/07/28 01:31:03, tkent wrote: > On 2014/07/28 01:30:15, tkent wrote: > > The core ...
6 years, 4 months ago (2014-07-28 17:09:40 UTC) #21
Habib Virji
On 2014/07/28 17:09:40, Habib Virji wrote: > On 2014/07/28 01:31:03, tkent wrote: > > On ...
6 years, 4 months ago (2014-07-28 17:13:38 UTC) #22
Habib Virji
On 2014/07/28 17:13:38, Habib Virji wrote: > On 2014/07/28 17:09:40, Habib Virji wrote: > > ...
6 years, 4 months ago (2014-07-31 03:54:53 UTC) #23
tkent
https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFormControlElement.cpp File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFormControlElement.cpp#newcode416 Source/core/html/HTMLFormControlElement.cpp:416: AtomicString dir = fastGetAttribute(dirAttr); Why do you refer to ...
6 years, 4 months ago (2014-08-08 01:41:14 UTC) #24
Habib Virji
On 2014/08/08 01:41:14, tkent wrote: > https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFormControlElement.cpp > File Source/core/html/HTMLFormControlElement.cpp (right): > > https://codereview.chromium.org/389343002/diff/220001/Source/core/html/HTMLFormControlElement.cpp#newcode416 > ...
6 years, 4 months ago (2014-08-08 08:30:16 UTC) #25
tkent
On 2014/08/08 08:30:16, Habib Virji wrote: > Your point is valid, reason I included was ...
6 years, 4 months ago (2014-08-08 09:21:09 UTC) #26
Habib Virji
On 2014/08/08 09:21:09, tkent wrote: > On 2014/08/08 08:30:16, Habib Virji wrote: > > Your ...
6 years, 4 months ago (2014-08-08 10:43:23 UTC) #27
tkent
https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFormControlElementTest.cpp File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFormControlElementTest.cpp#newcode14 Source/core/html/HTMLFormControlElementTest.cpp:14: using namespace blink; We may simply use blink namespace ...
6 years, 4 months ago (2014-08-11 01:45:49 UTC) #28
Habib Virji
Thanks, updated as per code review comments https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFormControlElementTest.cpp File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/240001/Source/core/html/HTMLFormControlElementTest.cpp#newcode14 Source/core/html/HTMLFormControlElementTest.cpp:14: using namespace ...
6 years, 4 months ago (2014-08-11 20:34:30 UTC) #29
tkent
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#newcode3450 Source/core/core.gypi:3450: 'html/HTMLFormControlElementTest.cpp', Sort alphabetically. https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFormControlElementTest.cpp File Source/core/html/HTMLFormControlElementTest.cpp (right): https://codereview.chromium.org/389343002/diff/260001/Source/core/html/HTMLFormControlElementTest.cpp#newcode54 ...
6 years, 4 months ago (2014-08-12 00:19:58 UTC) #30
Habib Virji
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#newcode3450 Source/core/core.gypi:3450: 'html/HTMLFormControlElementTest.cpp', On 2014/08/12 ...
6 years, 4 months ago (2014-08-12 07:56:17 UTC) #31
Habib Virji
The CQ bit was checked by habib.virji@samsung.com
6 years, 4 months ago (2014-08-12 07:56:30 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/280001
6 years, 4 months ago (2014-08-12 07:56:59 UTC) #33
Habib Virji
The CQ bit was checked by habib.virji@samsung.com
6 years, 4 months ago (2014-08-12 08:32:55 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/300001
6 years, 4 months ago (2014-08-12 08:33:22 UTC) #35
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink ...
6 years, 4 months ago (2014-08-12 09:41:59 UTC) #36
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-12 10:51:03 UTC) #37
commit-bot: I haz the power
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)
6 years, 4 months ago (2014-08-12 10:51:04 UTC) #38
Habib Virji
The CQ bit was checked by habib.virji@samsung.com
6 years, 4 months ago (2014-08-12 11:54:40 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/habib.virji@samsung.com/389343002/300001
6 years, 4 months ago (2014-08-12 11:55:00 UTC) #40
commit-bot: I haz the power
6 years, 4 months ago (2014-08-12 12:30:11 UTC) #41
Message was sent while issue was closed.
Change committed as 180069

Powered by Google App Engine
This is Rietveld 408576698