Daring Fireball long press highlights are unnecessarily inflated due to false illegibility
Created attachment 374739 [details] Patch
Comment on attachment 374739 [details] Patch Attachment 374739 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12798739 New failing tests: fast/text-indicator/text-indicator-with-low-contrast-text.html
Created attachment 374756 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Oh I forgot to use ahem
Comment on attachment 374739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374739&action=review r=me > Source/WebCore/platform/graphics/ColorUtilities.cpp:-107 > -float luminance(const FloatComponents& sRGBCompontents) This is my favorite part. > Source/WebCore/platform/graphics/ColorUtilities.cpp:123 > + float lighterLuminance = luminance(componentsA); > + float darkerLuminance = luminance(componentsB); > + > + if (lighterLuminance < darkerLuminance) > + std::swap(lighterLuminance, darkerLuminance); Can we be fancy? auto [ lighterLuminance, darkerLuminance ] = std::minmax(componentsA, componentsB);
I meant: auto [ lighterLuminance, darkerLuminance ] = std::minmax(luminance(componentsA), luminance(componentsB));
Created attachment 374792 [details] Patch
(In reply to Geoffrey Garen from comment #6) > I meant: > > auto [ lighterLuminance, darkerLuminance ] = > std::minmax(luminance(componentsA), luminance(componentsB)); Sure! But, you ACTUALLY meant auto [ darkerLuminance, lighterLuminance ] = std::minmax(luminance(componentsA), luminance(componentsB));
Created attachment 374793 [details] Patch
Attachment 374793 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/ColorUtilities.cpp:119: Extra space in capture list. [whitespace/brackets] [4] ERROR: Source/WebCore/platform/graphics/ColorUtilities.cpp:119: Extra space before [. [whitespace/brackets] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 374793 [details] Patch Attachment 374793 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12802905 New failing tests: fast/text-indicator/text-indicator-with-low-contrast-text.html
Created attachment 374797 [details] Archive of layout-test-results from ews105 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 374793 [details] Patch Attachment 374793 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12802996 New failing tests: fast/text-indicator/text-indicator-with-low-contrast-text.html
Created attachment 374798 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
That didn't start happening till the minmax. Too fancy.
Created attachment 374805 [details] Patch
One more shot with Geoff's version and then we go back to mine
Attachment 374805 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/ColorUtilities.cpp:119: Extra space in capture list. [whitespace/brackets] [4] ERROR: Source/WebCore/platform/graphics/ColorUtilities.cpp:119: Extra space before [. [whitespace/brackets] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 374805 [details] Patch Attachment 374805 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12803730 New failing tests: fast/text-indicator/text-indicator-with-low-contrast-text.html
Created attachment 374813 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Alright back to my version
Created attachment 374818 [details] Patch
Comment on attachment 374818 [details] Patch Clearing flags on attachment: 374818 Committed r247792: <https://trac.webkit.org/changeset/247792>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53516692>
Comment on attachment 374818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374818&action=review > Source/WebCore/testing/Internals.h:860 > if (useBoundingRectAndPaintAllContentForComplexRanges) > options = options | TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges; > + if (computeEstimatedBackgroundColor) > + options = options | TextIndicatorOptionComputeEstimatedBackgroundColor; > + if (respectTextColor) > + options = options | TextIndicatorOptionRespectTextColor; Surprised that |= is not used here. Is there a reason why?
(In reply to Darin Adler from comment #26) > Comment on attachment 374818 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374818&action=review > > > Source/WebCore/testing/Internals.h:860 > > if (useBoundingRectAndPaintAllContentForComplexRanges) > > options = options | TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges; > > + if (computeEstimatedBackgroundColor) > > + options = options | TextIndicatorOptionComputeEstimatedBackgroundColor; > > + if (respectTextColor) > > + options = options | TextIndicatorOptionRespectTextColor; > > Surprised that |= is not used here. Is there a reason why? Definitely no reason
Might need iOS-specific results, I'll see when it uploads its bits
(In reply to Tim Horton from comment #27) > (In reply to Darin Adler from comment #26) > > Comment on attachment 374818 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=374818&action=review > > > > > Source/WebCore/testing/Internals.h:860 > > > if (useBoundingRectAndPaintAllContentForComplexRanges) > > > options = options | TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges; > > > + if (computeEstimatedBackgroundColor) > > > + options = options | TextIndicatorOptionComputeEstimatedBackgroundColor; > > > + if (respectTextColor) > > > + options = options | TextIndicatorOptionRespectTextColor; > > > > Surprised that |= is not used here. Is there a reason why? > > Definitely no reason It should also totally be an OptionSet
(In reply to Tim Horton from comment #28) > Might need iOS-specific results, I'll see when it uploads its bits Here is the diff on iOS. Are iOS-specific results warranted? @@ -1,40 +1,40 @@ whiteTextWhiteBackground: NOT legible - 0 0 68 66 + 0 0 68 70 whiteTextGrayBackground: NOT legible - 0 0 68 66 + 0 0 68 70 whiteTextBlackBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 grayTextWhiteBackground: NOT legible - 0 0 68 66 + 0 0 68 70 grayTextGrayBackground: NOT legible - 0 0 68 66 + 0 0 68 70 grayTextBlackBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 lightGrayTextWhiteBackground: NOT legible - 0 0 68 66 + 0 0 68 70 lightGrayTextGrayBackground: NOT legible - 0 0 68 66 + 0 0 68 70 lightGrayTextBlackBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 darkGrayTextWhiteBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 darkGrayTextGrayBackground: NOT legible - 0 0 68 66 + 0 0 68 70 darkGrayTextBlackBackground: NOT legible - 0 0 68 66 + 0 0 68 70 blackTextWhiteBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 blackTextGrayBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19 blackTextBlackBackground: NOT legible - 0 0 68 66 + 0 0 68 70 dfTextDfBackground: legible - 0 0 68 18 - 0 48 36 18 + 0 0 68 19 + 0 51 36 19
(In reply to Russell Epstein from comment #30) > (In reply to Tim Horton from comment #28) > > Might need iOS-specific results, I'll see when it uploads its bits > > Here is the diff on iOS. Are iOS-specific results warranted? Yeah, that’s fine! Can you land baselines? Otherwise I will this evening
(In reply to Tim Horton from comment #31) > (In reply to Russell Epstein from comment #30) > > (In reply to Tim Horton from comment #28) > > > Might need iOS-specific results, I'll see when it uploads its bits > > > > Here is the diff on iOS. Are iOS-specific results warranted? > > Yeah, that’s fine! Can you land baselines? Otherwise I will this evening Not a problem.
(In reply to Russell Epstein from comment #32) > (In reply to Tim Horton from comment #31) > > (In reply to Russell Epstein from comment #30) > > > (In reply to Tim Horton from comment #28) > > > > Might need iOS-specific results, I'll see when it uploads its bits > > > > > > Here is the diff on iOS. Are iOS-specific results warranted? > > > > Yeah, that’s fine! Can you land baselines? Otherwise I will this evening > > Not a problem. Landed baselines in https://trac.webkit.org/changeset/247824/webkit
Thank you!