Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161963003:
Make autofill dropdown to obey text-align: left|right CSS property. (Closed)
|
Created:
4 years, 5 months ago by Gleb Lanbin Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, kinuko+watch, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake autofill dropdown to obey text-align: left|right CSS property.
This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component.
BUG=482339
Committed: https://crrev.com/69ee7354e94f121f04620ac8e1f9cfa84b31fbf7
Cr-Commit-Position: refs/heads/master@{#407596}
Patch Set 1 #Patch Set 2 : fix comments #
Total comments: 1
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 ========== to ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 ==========
glebl@chromium.org changed reviewers: + mathp@google.com
glebl@chromium.org changed reviewers: + mathp@chromium.org - mathp@google.com
glebl@chromium.org changed reviewers: + eae@chromium.org
glebl@chromium.org changed reviewers: - eae@chromium.org
this lgtm from the Autofill point of view but you'll have to find an owner in WebKit. I'm not familiar enough with the expectation that website developers have. Thanks!
glebl@chromium.org changed reviewers: + esprehn@chromium.org
Elliott, do you have time to review/approve this tiny patch? thanks in advance.
esprehn@chromium.org changed reviewers: + eae@chromium.org
Hmm RTL isn't quite the same thing as text-align: right. eae@ what do you think?
This conflates direcitonality with alignment in a way that is confusing. How about adding a wrapper method called something like alignmentForPopup that returns the alignment if set and if not falls back on directionForFormData? Then changing the popup code to use the new alignmentForPopup call?
On 2016/07/20 16:46:20, eae wrote: > This conflates direcitonality with alignment in a way that is confusing. > > How about adding a wrapper method called something like alignmentForPopup that > returns the alignment if set and if not falls back on directionForFormData? > > Then changing the popup code to use the new alignmentForPopup call? done.
Perfect, LGTM
Elliott, do you have any other comments? I still need an owner approval to submit this.
Are you sure this works? https://codereview.chromium.org/2161963003/diff/20001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/2161963003/diff/20001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1408: field->text_direction = base::i18n::LEFT_TO_RIGHT; This makes text-align always win over direction. I think all elements have text-align set and the default is left, so this will make this always left even for things in RTL?
On 2016/07/21 22:05:19, esprehn wrote: > Are you sure this works? > > https://codereview.chromium.org/2161963003/diff/20001/components/autofill/con... > File components/autofill/content/renderer/form_autofill_util.cc (right): > > https://codereview.chromium.org/2161963003/diff/20001/components/autofill/con... > components/autofill/content/renderer/form_autofill_util.cc:1408: > field->text_direction = base::i18n::LEFT_TO_RIGHT; > This makes text-align always win over direction. I think all elements have > text-align set and the default is left, so this will make this always left even > for things in RTL? yes, it works as discussed in the bug, i.e. if text-align is set then it will override 'direction'. the default text_align value is TASTART, not LEFT. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Com... this patch has been verified with existing unittests: https://cs.chromium.org/chromium/src/chrome/renderer/autofill/form_autofill_b... jsfiddle: https://jsfiddle.net/glanbin/spjf92hs/ and on a real world example: http://accounts.google.com/ServiceLogin?hl=ar
Elliott, just a friendly reminder that I'm waiting for your review.
Lgtm, sorry I'm delayed due to family
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2161963003/#ps20001 (title: "fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by glebl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 ========== to ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 ========== to ========== Make autofill dropdown to obey text-align: left|right CSS property. This changes WebFormControlElement::directionForFormData so it takes into account text-align: left|right while returning the text direction used in autofill dropdown component. BUG=482339 Committed: https://crrev.com/69ee7354e94f121f04620ac8e1f9cfa84b31fbf7 Cr-Commit-Position: refs/heads/master@{#407596} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/69ee7354e94f121f04620ac8e1f9cfa84b31fbf7 Cr-Commit-Position: refs/heads/master@{#407596} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
