Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220733012:
Display transparent text when it's being searched and selected. (Closed)
|
Created:
5 years, 5 months ago by Sk.kumar Modified:
5 years, 4 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, dshwang, pals, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDisplay transparent text when it's being searched and selected.
BUG=504712
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200488
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed Extra spaces. #Patch Set 3 : Added Layout Test #
Total comments: 4
Patch Set 4 : Incorporated comments. #
Total comments: 2
Patch Set 5 : Removed extra construction of textRun and updated the LayoutTest as suggested. #Patch Set 6 : Rebased and Fixed the build error #
Messages
Total messages: 40 (9 generated)
sk.kumar@samsung.com changed reviewers: + yosin@chromium.org
yosin@chromium.org changed reviewers: + pdr@chromium.org
I'm not familiar with painting code. pdr@, could you take look?
schenney@chromium.org changed reviewers: + schenney@chromium.org
Drive by. I don't see any problem with this, but pdr@ should give an opinion because in some sense this is a publicly visible change in behavior. I searched a little for how transparent text is treated by Google search indexing, and Google does indeed index the text, but penalizes the page. And we do find the text in "Find In Page", I presume. Hence I think that this is the right approach to fixing the issue. Basically we are providing visual feedback for something we are doing anyway. It has the side benefit of making it easier for users to find out when a site is trying to hide stuff.
I think this is a reasonable approach. Please add a test that fails without your patch and passes with it. https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... File Source/core/paint/InlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... Source/core/paint/InlineTextBoxPainter.cpp:312: void InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int length, const ComputedStyle& style) extra spaces around int length https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, false); We shouldn't add all of this code. Can you modify the existing marker paint code to detect the transparent color and switch to another color?
https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... File Source/core/paint/InlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... Source/core/paint/InlineTextBoxPainter.cpp:312: void InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int length, const ComputedStyle& style) On 2015/07/06 22:09:16, pdr wrote: > extra spaces around int length Done. https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, false); On 2015/07/06 22:09:16, pdr wrote: > We shouldn't add all of this code. Can you modify the existing marker paint code > to detect the transparent color and switch to another color? This is being done to maintain the flow of Paint. 1st - background and then 2nd- foreground. Also, at the point when the marker paint is executed, we don't have the textpainter, so first we have to create one and then use. Currently, we already have the textpainter at this point. So, it's easier to go through DocumentMarkerVector of type TextMatch and paint the text with color:transparent. Please let me know your opinion on this.
On 2015/07/06 22:09:16, pdr wrote: > I think this is a reasonable approach. > > Please add a test that fails without your patch and passes with it. > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > File Source/core/paint/InlineTextBoxPainter.cpp (right): > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > Source/core/paint/InlineTextBoxPainter.cpp:312: void > InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int > length, const ComputedStyle& style) > extra spaces around int length > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = > TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, > false); > We shouldn't add all of this code. Can you modify the existing marker paint code > to detect the transparent color and switch to another color? Regarding the test: It seems testRunner.findString() or document.execCommand("FindString", false, <searched_text>) follow different InlineTextBoxPainter::paint flow than the normal one. The text highlighting and text paint happens in the same manner as the normal selection. So, LayoutTests using above means doesn't look feasible right now. I request you to please help me in this regard.
On 2015/07/07 at 12:45:02, sk.kumar wrote: > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > File Source/core/paint/InlineTextBoxPainter.cpp (right): > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > Source/core/paint/InlineTextBoxPainter.cpp:312: void InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int length, const ComputedStyle& style) > On 2015/07/06 22:09:16, pdr wrote: > > extra spaces around int length > > Done. > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, false); > On 2015/07/06 22:09:16, pdr wrote: > > We shouldn't add all of this code. Can you modify the existing marker paint code > > to detect the transparent color and switch to another color? > > This is being done to maintain the flow of Paint. > 1st - background and then 2nd- foreground. > Also, at the point when the marker paint is executed, we don't have the textpainter, so first we have to create one and then use. > Currently, we already have the textpainter at this point. So, it's easier to go through DocumentMarkerVector of type TextMatch and paint the text with color:transparent. > Please let me know your opinion on this. The existing function, paintDocumentMarkers, already maintains the flow of paint, right? I think you want to modify that function to clip out the text. Maybe we should talk about how you want this to work. For transparent text on a black background, what should be drawn? For white text on a white background, what should be drawn? For green text on a green background, what should be drawn?
On 2015/07/07 at 13:07:26, sk.kumar wrote: > On 2015/07/06 22:09:16, pdr wrote: > > I think this is a reasonable approach. > > > > Please add a test that fails without your patch and passes with it. > > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > File Source/core/paint/InlineTextBoxPainter.cpp (right): > > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > Source/core/paint/InlineTextBoxPainter.cpp:312: void > > InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int > > length, const ComputedStyle& style) > > extra spaces around int length > > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = > > TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, > > false); > > We shouldn't add all of this code. Can you modify the existing marker paint code > > to detect the transparent color and switch to another color? > > Regarding the test: > It seems testRunner.findString() or document.execCommand("FindString", false, <searched_text>) > follow different InlineTextBoxPainter::paint flow than the normal one. > The text highlighting and text paint happens in the same manner as the normal selection. > So, LayoutTests using above means doesn't look feasible right now. > > I request you to please help me in this regard. You may have to add an API to set the find-in-page result via Internals.cpp. You may want to look for other changes that affected find-in-page and see how their code was tested.
On 2015/07/07 21:53:25, pdr wrote: > On 2015/07/07 at 12:45:02, sk.kumar wrote: > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > File Source/core/paint/InlineTextBoxPainter.cpp (right): > > > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > Source/core/paint/InlineTextBoxPainter.cpp:312: void > InlineTextBoxPainter::paintTextMatchMarkerText(TextPainter textPainter, int > length, const ComputedStyle& style) > > On 2015/07/06 22:09:16, pdr wrote: > > > extra spaces around int length > > > > Done. > > > > > https://codereview.chromium.org/1220733012/diff/1/Source/core/paint/InlineTex... > > Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = > TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, > false); > > On 2015/07/06 22:09:16, pdr wrote: > > > We shouldn't add all of this code. Can you modify the existing marker paint > code > > > to detect the transparent color and switch to another color? > > > > This is being done to maintain the flow of Paint. > > 1st - background and then 2nd- foreground. > > Also, at the point when the marker paint is executed, we don't have the > textpainter, so first we have to create one and then use. > > Currently, we already have the textpainter at this point. So, it's easier to > go through DocumentMarkerVector of type TextMatch and paint the text with > color:transparent. > > Please let me know your opinion on this. > > The existing function, paintDocumentMarkers, already maintains the flow of > paint, right? I think you want to modify that function to clip out the text. > > Maybe we should talk about how you want this to work. > For transparent text on a black background, what should be drawn? > For white text on a white background, what should be drawn? > For green text on a green background, what should be drawn? paintDocumentMarkers -> paintTextMatchMarker gets triggered form InlineTextBoxPainter.cpp:paint() - line:159 before the text(foreground) paint happens. Also, paintTextMatchMarker doesn't have textpainter, so we have to create one there if we want to paint the transparent text there itself. Moreover, for the text with color transparent we have to avoid textpainter.paint call at line no:221 otherwise, it will again paint the text with transparent color. So, I feel it's better to maintain the current flow of the paint. I don't see any problem with transparent text on black background or white text on white background etc. in Find In Page Scenario, because, In these cases, when the text will be searched the background will become orange/yellow. So the text will be visible in these cases. However, I see problem with orange/yellow text with orange/yellow background (or any color text with orangle/yellow background - here selection won't be visible) because there is no invert of highlight color in paintTextMatchMarker. This Problem is already there in latest chrome. I will upload a patch to fix this behavior. Please let me know your suggestions.
pdr@, I have added the layout test. Please take a look.
pdr@chromium.org changed reviewers: + wkorman@chromium.org
+wkorman, our selection correction expert.
Thanks for adding that testcase, it looks good. https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... File Source/core/paint/InlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... Source/core/paint/InlineTextBoxPainter.cpp:224: paintTextMatchMarkerText(textPainter, length, styleToUse); Where is the black color of the text coming from? https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, false); We generally try not to duplicate complex logic like this because it leads to broken edge cases. For example, a developer fixing a bug in paintDocumentMarkers below might not know to fix paintTextMatchMarkerText too. Maybe this is an exception but can you think of a way to modify paintDocumentMarkers() to work instead? https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... File Source/core/paint/InlineTextBoxPainter.h (right): https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... Source/core/paint/InlineTextBoxPainter.h:8: #include "core/paint/TextPainter.h" I think you can just forward declare this ("class TextPainter" below "class LayoutTextCombine") instead of including another header. https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... Source/core/paint/InlineTextBoxPainter.h:33: void paintTextMatchMarkerText(TextPainter, int, const ComputedStyle&); TextPainter is a large class so we shouldn't pass it by value. For a large object like TextPainter, we'd want to use "TextPainter&" here. Unless the meaning is trivial (like TextPainter and ComputedStyle), we should include a name for all variables. In this case, just change "int" to "int length". In general we shouldn't pass Painter classes around though, as they are meant to be short-lived for simplicity. If you'd like, in a followup patch, we could add "STACK_ALLOCATED();" and "WTF_MAKE_NONCOPYABLE(...)" to all of the painter classes to enforce this.
pdr@, I have incorporated the changes suggested by you. Please take a look. > https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... > Source/core/paint/InlineTextBoxPainter.cpp:224: > paintTextMatchMarkerText(textPainter, length, styleToUse); > Where is the black color of the text coming from? Here, I have created the textStyle with forceBlacktext. > https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... > Source/core/paint/InlineTextBoxPainter.cpp:317: TextPainter::Style textStyle = > TextPainter::textPaintingStyle(m_inlineTextBox.layoutObject(), style, true, > false); > We generally try not to duplicate complex logic like this because it leads to > broken edge cases. For example, a developer fixing a bug in paintDocumentMarkers > below might not know to fix paintTextMatchMarkerText too. Maybe this is an > exception but can you think of a way to modify paintDocumentMarkers() to work > instead? Moved the functionality to highlight the text with color:transparent in paintTextMatchMarker().
On 2015/07/21 at 06:53:06, sk.kumar wrote: > pdr@, I have incorporated the changes suggested by you. > Please take a look. > > > https://codereview.chromium.org/1220733012/diff/40001/Source/core/paint/Inlin... > > Source/core/paint/InlineTextBoxPainter.cpp:224: > > paintTextMatchMarkerText(textPainter, length, styleToUse); > > Where is the black color of the text coming from? > > Here, I have created the textStyle with forceBlacktext. It looks like you were actually passing true for useTextAsClip, not forceBlackText. I've filed crbug.com/512587 to clean this up so it's easier to follow.
https://codereview.chromium.org/1220733012/diff/60001/LayoutTests/fast/repain... File LayoutTests/fast/repaint/text-match-transparent-text.html (right): https://codereview.chromium.org/1220733012/diff/60001/LayoutTests/fast/repain... LayoutTests/fast/repaint/text-match-transparent-text.html:12: highlightRange('1', 0, 6, true); Can you modify this to only select a small range, such as "ndM" from "findMe"? https://codereview.chromium.org/1220733012/diff/60001/Source/core/paint/Inlin... File Source/core/paint/InlineTextBoxPainter.cpp (right): https://codereview.chromium.org/1220733012/diff/60001/Source/core/paint/Inlin... Source/core/paint/InlineTextBoxPainter.cpp:821: TextRun textRun = m_inlineTextBox.constructTextRun(style, font, string, length, 0); You're constructing a new textrun here. Why doesn't the textrun from above (line 806) work?
pdr@, please take a look. > https://codereview.chromium.org/1220733012/diff/60001/LayoutTests/fast/repain... > LayoutTests/fast/repaint/text-match-transparent-text.html:12: > highlightRange('1', 0, 6, true); > Can you modify this to only select a small range, such as "ndM" from "findMe"? Done. > https://codereview.chromium.org/1220733012/diff/60001/Source/core/paint/Inlin... > Source/core/paint/InlineTextBoxPainter.cpp:821: TextRun textRun = > m_inlineTextBox.constructTextRun(style, font, string, length, 0); > You're constructing a new textrun here. Why doesn't the textrun from above (line > 806) work? Done.
On 2015/07/22 at 05:10:59, sk.kumar wrote: > pdr@, please take a look. > > > https://codereview.chromium.org/1220733012/diff/60001/LayoutTests/fast/repain... > > LayoutTests/fast/repaint/text-match-transparent-text.html:12: > > highlightRange('1', 0, 6, true); > > Can you modify this to only select a small range, such as "ndM" from "findMe"? > Done. > > > https://codereview.chromium.org/1220733012/diff/60001/Source/core/paint/Inlin... > > Source/core/paint/InlineTextBoxPainter.cpp:821: TextRun textRun = > > m_inlineTextBox.constructTextRun(style, font, string, length, 0); > > You're constructing a new textrun here. Why doesn't the textrun from above (line > > 806) work? > Done. Thanks! I think this looks good in terms of implementation now. Lets wait for a couple days to see if we can get a response from a UX person on the bug.
On 2015/07/22 06:56:12, pdr wrote: > Thanks! I think this looks good in terms of implementation now. Lets wait for a > couple days to see if we can get a response from a UX person on the bug. pdr@, I don't see any response from UX person on the bug. it's been 6-7 days, should we wait for more days??
On 2015/07/28 at 13:18:25, sk.kumar wrote: > On 2015/07/22 06:56:12, pdr wrote: > > Thanks! I think this looks good in terms of implementation now. Lets wait for a > > couple days to see if we can get a response from a UX person on the bug. > > pdr@, I don't see any response from UX person on the bug. > it's been 6-7 days, should we wait for more days?? LGTM, lets go ahead with this. Thanks for your patience.
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220733012/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220733012/80001
The CQ bit was unchecked by commit-bot@chromium.org
The author sk.kumar@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2015/08/12 at 05:20:26, commit-bot wrote: > The author sk.kumar@samsung.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA. sk.kumar@samsung.com, it looks like we'll need you to sign the CLA. I believe Laszlo Gombos handles these for Samsung.
On 2015/08/12 05:22:55, pdr wrote: > On 2015/08/12 at 05:20:26, commit-bot wrote: > > The author mailto:sk.kumar@samsung.com has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. > > mailto:sk.kumar@samsung.com, it looks like we'll need you to sign the CLA. I believe > Laszlo Gombos handles these for Samsung. pdr@, i will get the CLA signed. Once done, i will update. Thanks, Shashi
On 2015/08/12 at 05:29:39, sk.kumar wrote: > On 2015/08/12 05:22:55, pdr wrote: > > On 2015/08/12 at 05:20:26, commit-bot wrote: > > > The author mailto:sk.kumar@samsung.com has not signed Google Contributor License > > Agreement. Please visit https://cla.developers.google.com to sign and manage > > CLA. > > > > mailto:sk.kumar@samsung.com, it looks like we'll need you to sign the CLA. I believe > > Laszlo Gombos handles these for Samsung. > > pdr@, i will get the CLA signed. Once done, i will update. > Thanks, > Shashi Feel free to click commit once the CLA goes through.
On 2015/08/12 05:34:07, pdr wrote: > Feel free to click commit once the CLA goes through. Added Shashi to the CLA, setting commit bit.
The CQ bit was checked by l.gombos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220733012/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220733012/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
On 2015/08/12 at 16:07:59, commit-bot wrote: > Try jobs failed on following builders: > linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...) sk.kumar, it looks like your patch is failing to compile. I think eae's updates to the text painting code last week caused this. Can you update your checkout and re-upload? I usually do something like this: cd third_party/WebKit git checkout origin/master git pull origin master git checkout [this patch] git rebase origin/master [ figure out git's silly rebase thing ] git cl upload
On 2015/08/13 05:26:03, pdr wrote: > On 2015/08/12 at 16:07:59, commit-bot wrote: > > Try jobs failed on following builders: > > linux_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...) > > sk.kumar, it looks like your patch is failing to compile. I think eae's updates > to the text painting code last week caused this. Can you update your checkout > and re-upload? > > I usually do something like this: > cd third_party/WebKit > git checkout origin/master > git pull origin master > git checkout [this patch] > git rebase origin/master > [ figure out git's silly rebase thing ] > git cl upload pdr@, Please review.
The CQ bit was checked by pdr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220733012/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1220733012/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200488 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
