Bug 200064 – Daring Fireball long press highlights are unnecessarily inflated due to false illegibility

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Daring Fireball long press highlights are unnecessarily inflated due to false illegibility

[Tim Horton]

Created attachment 374739 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

Oh I forgot to use ahem

[Geoffrey Garen]

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);

[Geoffrey Garen]

I meant:

auto [ lighterLuminance, darkerLuminance ] = std::minmax(luminance(componentsA), luminance(componentsB));

[Tim Horton]

Created attachment 374792 [details]
Patch

[Tim Horton]

(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));

[Tim Horton]

Created attachment 374793 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

That didn't start happening till the minmax. Too fancy.

[Tim Horton]

Created attachment 374805 [details]
Patch

[Tim Horton]

One more shot with Geoff's version and then we go back to mine

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Tim Horton]

Alright back to my version

[Tim Horton]

Created attachment 374818 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374818 [details]
Patch

Clearing flags on attachment: 374818

Committed r247792: <https://trac.webkit.org/changeset/247792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53516692>

[Darin Adler]

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?

[Tim Horton]

(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

[Tim Horton]

Might need iOS-specific results, I'll see when it uploads its bits

[Tim Horton]

(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

[Russell Epstein]

(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

[Tim Horton]

(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

[Russell Epstein]

(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.

[Russell Epstein]

(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

[Tim Horton]

Thank you!