Bug 188891 - REGRESSION (r232991): Switching to dark mode in Mail does not update the message view to be transparent
Summary: REGRESSION (r232991): Switching to dark mode in Mail does not update the mess...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-23 10:38 PDT by Timothy Hatcher
Modified: 2018-12-19 17:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.19 KB, patch)
2018-08-23 10:41 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2018-08-23 10:38:27 PDT
<rdar://problem/42344352>
Comment 1 Radar WebKit Bug Importer 2018-08-23 10:38:45 PDT
<rdar://problem/43648659>
Comment 2 Timothy Hatcher 2018-08-23 10:41:00 PDT
Created attachment 347934 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-08-23 10:41:47 PDT
Comment on attachment 347934 [details]
Patch

Needs a test.
Comment 4 Timothy Hatcher 2018-09-18 10:41:10 PDT
Comment on attachment 347934 [details]
Patch

Yesterday and this morning I tried to write an API test for this. I was not able to recreate the bug in the test harness -- only in Mail. 

In the test runner, rootBackgroundColorOrTransparencyChanged() ends upon getting called with extendedBackgroundColorChanged as true and then false in short succession. So the bad early return was blocking updateRootLayerConfiguration() first, but then ends up getting it scheduled soon after then the color does not change.

Maybe you have a suggestion on how to write a test for this, but I'm at a loss.

Can we just get this landed? Reverting the bad part of r232991 and returning us to the old behavior of no early return.
Comment 5 Timothy Hatcher 2018-09-18 10:42:11 PDT
Typo fix: "...scheduled soon after when the color does not change."
Comment 6 Simon Fraser (smfr) 2018-09-18 11:11:42 PDT
You should be able to change view transparency in a layout test, then dump the graphics layer tree to show the "contentsOpaque" on the page tile layers (assuming we can dump them. if not, make a way to dump them!).
Comment 7 Timothy Hatcher 2018-09-18 11:48:50 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> You should be able to change view transparency in a layout test, then dump
> the graphics layer tree to show the "contentsOpaque" on the page tile layers
> (assuming we can dump them. if not, make a way to dump them!).

That is what I was doing, but with an API test. And it wasn't triggering the problem observed in Mail.
Comment 8 Timothy Hatcher 2018-09-19 12:46:50 PDT
I get the same results in a layout test as I did with the API test -- I can't reproduce  the bug. The contentsOpaque flag always get cleared.

Before:
(GraphicsLayer
  (anchor 0.00 0.00)
  (bounds 785.00 1024.00)
  (children 1
    (GraphicsLayer
      (bounds 785.00 1024.00)
      (contentsOpaque 1)
      (tile cache coverage 0, 0 785 x 1024)
      (tile size 785 x 512)
      (top left tile 0, 0 tiles grid 1 x 2)
      (in window 1)
    )
  )
)

After setDrawsBackground(false):
(GraphicsLayer
  (anchor 0.00 0.00)
  (bounds 785.00 1024.00)
  (children 1
    (GraphicsLayer
      (bounds 785.00 1024.00)
      (tile cache coverage 0, 0 785 x 1024)
      (tile size 785 x 512)
      (top left tile 0, 0 tiles grid 1 x 2)
      (in window 1)
    )
  )
)
Comment 9 Timothy Hatcher 2018-09-19 12:49:40 PDT
My test used an internals.setDrawsBackground(false) that triggers the same code path Mail is using.
Comment 10 WebKit Commit Bot 2018-12-19 17:28:12 PST
Comment on attachment 347934 [details]
Patch

Clearing flags on attachment: 347934

Committed r239414: <https://trac.webkit.org/changeset/239414>
Comment 11 WebKit Commit Bot 2018-12-19 17:28:14 PST
All reviewed patches have been landed.  Closing bug.