Closed Bug 1047829 Opened 5 years ago Closed 5 years ago

Set a background-color on the screenshot-overlay

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 2 obsolete files)

I fixed a positioning issue with the screenshot-overlay in this version.
Attachment #8466645 - Attachment is obsolete: true
Attachment #8466645 - Flags: review?(kgrandon)
Attachment #8466659 - Flags: review?(kgrandon)
Nice work, can confirm this works - this also sets the colour of the flashing in bug 1040087, but only at the top - you can still observe the flashing at the bottom.

Given that this area appears where the toolbar is meant to be, I'd assert that this colour should be the background colour of the system tray and not white - but white is better than black (unless the page and the chrome had a black background - then this would look as weird as it does pre-patch... But is an unlikely combination right now).

Would it be much work to change this to the system tray colour? I think that would provide better results and stop us from having to tackle this again in the future.
Comment on attachment 8466659 [details] [diff] [review]
background-color-screenshot-overlay.patch

Review of attachment 8466659 [details] [diff] [review]:
-----------------------------------------------------------------

+1 to what Chris said. Shouldn't we leverage the same 'light/dark' classes that we do in app_chrome.js? Looks good though!
Attachment #8466659 - Flags: review?(kgrandon)
(In reply to Chris Lord [:cwiiis] from comment #3)
> Would it be much work to change this to the system tray colour? I think that
> would provide better results and stop us from having to tackle this again in
> the future.

I guess I'm getting confused by terminology here. What do you refer to as the "system tray"? Is that not rocketbar/the app title?
So in this patch:
 - default color is white for non scrollable
 - default color is unset for homescreen window
 - for browser scrollable window, default color is the default color of the chrome
 - for window with a theme color set, the color is the color of the theme.
Attachment #8466659 - Attachment is obsolete: true
Attachment #8466762 - Flags: review?(kgrandon)
Comment on attachment 8466762 [details] [diff] [review]
background-color-screenshot-overlay.patch

Review of attachment 8466762 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I guess it seems fine if web content can not influence theme-color yet.
Attachment #8466762 - Flags: review?(kgrandon) → review+
(In reply to Kevin Grandon :kgrandon from comment #7)
> Comment on attachment 8466762 [details] [diff] [review]
> background-color-screenshot-overlay.patch
> 
> Review of attachment 8466762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I guess it seems fine if web content can not influence theme-color yet.

It works for theme-color if web content changed it!
https://github.com/mozilla-b2g/gaia/commit/c987645e91a87615e2faf3c0a76e114c0a5009b3
Assignee: nobody → 21
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1071235
No longer depends on: 1071235
Depends on: 1071235
You need to log in before you can comment on or make changes to this bug.