WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
200064
Daring Fireball long press highlights are unnecessarily inflated due to false illegibility
https://bugs.webkit.org/show_bug.cgi?id=200064
Summary
Daring Fireball long press highlights are unnecessarily inflated due to false...
Tim Horton
Reported
2019-07-23 17:25:03 PDT
Daring Fireball long press highlights are unnecessarily inflated due to false illegibility
Attachments
Patch
(15.79 KB, patch)
2019-07-23 17:25 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews214 for win-future
(13.60 MB, application/zip)
2019-07-23 20:40 PDT
,
EWS Watchlist
no flags
Details
Patch
(15.73 KB, patch)
2019-07-24 11:54 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(15.62 KB, patch)
2019-07-24 12:25 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2
(2.93 MB, application/zip)
2019-07-24 13:18 PDT
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews100 for mac-highsierra
(3.20 MB, application/zip)
2019-07-24 13:33 PDT
,
EWS Watchlist
no flags
Details
Patch
(15.63 KB, patch)
2019-07-24 14:06 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews101 for mac-highsierra
(3.18 MB, application/zip)
2019-07-24 14:56 PDT
,
EWS Watchlist
no flags
Details
Patch
(15.72 KB, patch)
2019-07-24 15:13 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2019-07-23 17:25:11 PDT
Created
attachment 374739
[details]
Patch
EWS Watchlist
Comment 2
2019-07-23 20:40:42 PDT
Comment hidden (obsolete)
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
Comment 3
2019-07-23 20:40:44 PDT
Comment hidden (obsolete)
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
Comment 4
2019-07-24 09:50:26 PDT
Oh I forgot to use ahem
Geoffrey Garen
Comment 5
2019-07-24 11:20:27 PDT
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
Comment 6
2019-07-24 11:21:12 PDT
I meant: auto [ lighterLuminance, darkerLuminance ] = std::minmax(luminance(componentsA), luminance(componentsB));
Tim Horton
Comment 7
2019-07-24 11:54:36 PDT
Created
attachment 374792
[details]
Patch
Tim Horton
Comment 8
2019-07-24 12:16:48 PDT
(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
Comment 9
2019-07-24 12:25:19 PDT
Created
attachment 374793
[details]
Patch
EWS Watchlist
Comment 10
2019-07-24 12:29:06 PDT
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 11
2019-07-24 13:18:12 PDT
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
Comment 12
2019-07-24 13:18:14 PDT
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 13
2019-07-24 13:33:25 PDT
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
Comment 14
2019-07-24 13:33:26 PDT
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
Comment 15
2019-07-24 13:48:10 PDT
That didn't start happening till the minmax. Too fancy.
Tim Horton
Comment 16
2019-07-24 14:06:03 PDT
Created
attachment 374805
[details]
Patch
Tim Horton
Comment 17
2019-07-24 14:06:18 PDT
One more shot with Geoff's version and then we go back to mine
EWS Watchlist
Comment 18
2019-07-24 14:08:14 PDT
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 19
2019-07-24 14:56:27 PDT
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
Comment 20
2019-07-24 14:56:28 PDT
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
Comment 21
2019-07-24 15:12:19 PDT
Alright back to my version
Tim Horton
Comment 22
2019-07-24 15:13:09 PDT
Created
attachment 374818
[details]
Patch
WebKit Commit Bot
Comment 23
2019-07-24 15:36:19 PDT
Comment on
attachment 374818
[details]
Patch Clearing flags on attachment: 374818 Committed
r247792
: <
https://trac.webkit.org/changeset/247792
>
WebKit Commit Bot
Comment 24
2019-07-24 15:36:21 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 25
2019-07-24 15:38:26 PDT
<
rdar://problem/53516692
>
Darin Adler
Comment 26
2019-07-24 16:48:51 PDT
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
Comment 27
2019-07-24 16:50:21 PDT
(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
Comment 28
2019-07-24 16:51:07 PDT
Might need iOS-specific results, I'll see when it uploads its bits
Tim Horton
Comment 29
2019-07-24 16:51:41 PDT
(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
Comment 30
2019-07-25 09:49:31 PDT
(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
Comment 31
2019-07-25 10:04:42 PDT
(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
Comment 32
2019-07-25 10:08:17 PDT
(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
Comment 33
2019-07-25 10:25:32 PDT
(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
Comment 34
2019-07-25 14:57:53 PDT
Thank you!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug