Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1557563002:
Fix for overlapping of cursor with page number in page selector. (Closed)
DescriptionUpdating the font size for 'host' to accommodate cursor.
BUG=572046
Committed: https://crrev.com/7a43d04f7ce9b9e5678c6c6537862e3c464d14bd
Cr-Commit-Position: refs/heads/master@{#369084}
Patch Set 1 #Patch Set 2 : Changes as per review comments. #
Total comments: 4
Patch Set 3 : Changes as per the review comments. #
Total comments: 1
Patch Set 4 : Changes as per review comments. #
Total comments: 3
Patch Set 5 : Changing font size to 68.75%. #Patch Set 6 : Changing the font size to 76.5%. #Messages
Total messages: 29 (10 generated)
Description was changed from ========== Making text-align 'start' for input element to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ========== to ========== Making text-align 'start' for input element to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ==========
Description was changed from ========== Making text-align 'start' for input element to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ========== to ========== Making text-align 'start' for input element to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ==========
deepak.m1@samsung.com changed reviewers: + raymes@chromium.org
PTAL
raymes@chromium.org changed reviewers: + tsergeant@chromium.org
+tsergeant
On 2016/01/03 at 23:52:17, raymes wrote: > +tsergeant This is not a sufficient solution, since it will change the alignment of the text in the top bar for PDFs with a lot of pages. A better solution might be to change the width of the input field?
Description was changed from ========== Making text-align 'start' for input element to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ========== to ========== Making text-align 'center' for input element, and increasing the size of pageselector by 1 'ch' to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ==========
@tsergeant I have done changes in $.pageselector.style.width by 1 'ch' to take care of cursor in the input box. After that cursor appears somewhat proper, But I feel we need to set "text-align: center" also, so that cursor appears at better position. With this Page number will be centered and cursor position is also proper. PTAL.
This seems like a weird rounding issue with the measurement in Blink itself. I've tried a lot of different measurements, some work, some do not: 8px: Works 0.5em: Works 0.55em: Does not work 0.6em: Works ... 1.2em: Works 1.8em: Doesn't work I would be happy to switch to 0.6em * numDigits, which is what we used to use (https://chromium.googlesource.com/chromium/src/+/f693f2575ec0a37917b1c8c42953... even that doesn't work with 3 digits. https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:31: text-align: center; Again, changing the alignment is something we want to avoid https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js (right): https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:45: this.$['pagelength-spacer'].style.width = numDigits + 'ch'; Any changes to line 42 need to be made here as well, to ensure the selector stays centered.
Description was changed from ========== Making text-align 'center' for input element, and increasing the size of pageselector by 1 'ch' to avoid cursor overlap with the page number in viewer-page-selector. BUG=572046 ========== to ========== Updating the font size for 'host' and #slash pagelength Now pagelength have more fontsize than host. BUG=572046 ==========
@tsergeant exchanging the font-size for 'host' and #slash pagelength works fine with 3 digit page number also. Earlier also font-size of pagelength is greater than the pageselector. https://codereview.chromium.org/1312373002/diff/80001/chrome/browser/resource... PTAL https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:31: text-align: center; On 2016/01/06 01:58:59, tsergeant wrote: > Again, changing the alignment is something we want to avoid Done. https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js (right): https://codereview.chromium.org/1557563002/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.js:45: this.$['pagelength-spacer'].style.width = numDigits + 'ch'; On 2016/01/06 01:58:59, tsergeant wrote: > Any changes to line 42 need to be made here as well, to ensure the selector > stays centered. Done.
https://codereview.chromium.org/1557563002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:7: font-size: 81.25%; This is a good workaround. The only problem is that it makes the input box slightly smaller, which means large numbers can be cut off. How about you make it slightly larger instead? (Say, 94.4%, which corresponds to 17px).
Description was changed from ========== Updating the font size for 'host' and #slash pagelength Now pagelength have more fontsize than host. BUG=572046 ========== to ========== Updating the font size for 'host' to accommodate cursor. BUG=572046 ==========
@tsergeant Thanks for review. Changes done as suggested. It is working fine. PTAL
https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:51: font-size: 81.25%; Please decrease this so the size isn't changed by this patch (Current size is 13px)
@tsergeant PTAL https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:51: font-size: 81.25%; On 2016/01/11 04:48:16, tsergeant wrote: > Please decrease this so the size isn't changed by this patch (Current size is > 13px) making 75% corresponds to 12px cuts numbers, so making it 68.75% corresponds to 11px, It works in all cases.
https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... File chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css (right): https://codereview.chromium.org/1557563002/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/elements/viewer-page-selector/viewer-page-selector.css:51: font-size: 81.25%; On 2016/01/11 at 05:24:31, Deepak wrote: > On 2016/01/11 04:48:16, tsergeant wrote: > > Please decrease this so the size isn't changed by this patch (Current size is > > 13px) > > making 75% corresponds to 12px cuts numbers, > so making it 68.75% corresponds to 11px, It works in all cases. Setting it to 76.5% corresponds to 13px, which works for me in all cases and doesn't change the size?
@tsergeant yes, 76.5% font size is working well. Changes done. PTAL
lgtm!
On 2016/01/12 04:12:48, tsergeant wrote: > lgtm! @tsergeant Thanks. @raymes PTAL
lgtm
On 2016/01/12 23:52:20, raymes wrote: > lgtm Thankyou Raymes.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1557563002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1557563002/100001
Message was sent while issue was closed.
Description was changed from ========== Updating the font size for 'host' to accommodate cursor. BUG=572046 ========== to ========== Updating the font size for 'host' to accommodate cursor. BUG=572046 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Updating the font size for 'host' to accommodate cursor. BUG=572046 ========== to ========== Updating the font size for 'host' to accommodate cursor. BUG=572046 Committed: https://crrev.com/7a43d04f7ce9b9e5678c6c6537862e3c464d14bd Cr-Commit-Position: refs/heads/master@{#369084} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7a43d04f7ce9b9e5678c6c6537862e3c464d14bd Cr-Commit-Position: refs/heads/master@{#369084} |
