When Modal Overlays are layered, only bottom layer has a dark background (fix #4808) #4820

willy-b · 2016-10-15T06:47:08Z

Submitted a ticket for my issue if one did not already exist. (fixes background color of modal window on ledger advanced settings gets darker by clicking backup wallet button #4808)
Used Github auto-closing keywords in the commit message.
Ran git rebase -i to squash commits (if needed).

Test Plan:

Open Brave, navigate to Preferences->Payments tab
Enable Payments
Click "Advanced Settings"
Observe background (page outside modal) is darkened.
Click "Backup Wallet"
Observe background does not get darker.

bbondy · 2016-10-18T15:38:54Z

Could you provide before / after screenshots?

willy-b · 2016-10-26T21:36:32Z

@bbondy, yep!

Before:

After:

luixxiul · 2016-11-23T03:52:13Z

Viewing the code I think setting background:none to .modal + .modal is simpler.

willy-b · 2016-11-23T04:46:49Z

@luixxiul agree. I just tested this and replaced my commit in this PR with your solution by force-push.

bsclifton · 2016-11-26T07:32:16Z

@willy-b One slight problem with this PR (sorry it hadn't been looked at before ☹️)

If the top level modal is smaller than the previous one / ones, you can see that they aren't being grayed out.

In this case, I had to hack the DOM to get this showing... but if the child window is smaller, this would be a problem. I simulated this by cracking open the dev tools and removing the main content div from the "recover your wallet" page

willy-b · 2016-11-27T22:30:16Z

good catch. looking into this now

willy-b · 2016-11-28T06:56:48Z

hey @bsclifton, after trying a bunch of CSS-only solutions, I fixed this by adding some logic to keep track of all the ModalOverlay instances and ensure only the top-most one has a last class which gives it a dark background. If you can recommend a CSS-only way, please do.

It's a little trickier than it looks (:last-child , :last-of-type won't work here since modal/non-modal divs are mixed together as siblings). Still seems like there should be a simple opposite of .modal + .modal { background: transparent } (which successfully ensures only the first modal has a dark background) that would work for ensuring the last modal would have a dark background here, but I couldn't find it.

luixxiul · 2016-11-28T06:58:33Z

If div[class^="modal"]:last-of-type could work, that would be the best solution, but it doesn't.

bsclifton · 2016-11-28T23:15:25Z

@willy-b it does look good for Advanced settings... but unfortunately, if you go to the Add funds screen and then hit either the QR code or the add with debit/credit (assuming those show up for you), they'll either have a missing overlay or a super dark overlay ☹️

willy-b · 2016-11-29T20:59:16Z

@bsclifton: there is now only one modal background in view for all cases and the QR popup background is styled correctly.

the background for the Coinbase iframe modal is still a bit darker because the background is actually from Coinbase's site which uses a darker gray. not sure what the best approach there is, feel free to advise!

bsclifton · 2016-12-02T17:40:32Z

@willy-b sorry it's taken me a few days to get back to you; I tried this out...\

overlay looks great between pages 😄
unfortunately, there is a flicker when going from add funds to the buy widget. Can you update this to keep the overlay (which would make it even darker?) That would prevent this flash

Regarding the iframe styles, the coloring is in the body tag for the buy widget. You might be able to select it with document.querySelectAll() and then update the object returned's style attribute (removing background-color or replacing with transparent).

willy-b · 2016-12-04T06:49:27Z

Hey @bsclifton, I made the change you asked for, but I just noticed @luixxiul has some changes on the way that may obsolete this (see #6009 (comment)).

@luixxiul you're fixing #4808 in PR #6009 then?

willy-b · 2016-12-04T06:53:21Z

Didn't mean to step on your toes @luixxiul, didn't see your changes.

luixxiul · 2016-12-04T06:58:34Z

@willy-b No, I cannot find a solution by myself. Please go ahead :-)

willy-b · 2016-12-04T07:04:55Z

Sorry. I'm confused. Is this issue supposed to be fixed in commit https://github.com/luixxiul/browser-laptop/commit/2327e9eca53eb4a637dd4c3e3a2a1d1f613c2fbc#commitcomment-19937858?

willy-b · 2016-12-04T07:08:06Z

This looks fixed in your screenshots, let's close this PR.

willy-b · 2016-12-04T07:28:57Z

OK, I just tested your commit @luixxiul https://github.com/luixxiul/browser-laptop/commit/2327e9eca53eb4a637dd4c3e3a2a1d1f613c2fbc#commitcomment-19937858 , and it looks perfect for the recovery dialog. The QR code popup still has a double overlay, which is not present in this PR, but I'm sure you can fix that too. The results are otherwise identical and your solution is MUCH cleaner (all CSS/LESS). You guys (@bsclifton, @luixxiul) make the call. Thanks!

bsclifton · 2016-12-04T08:41:32Z

@willy-b how about let's just keep this on the back-burner until @luixxiul's PR is accepted and then we'll revisit?

luixxiul · 2016-12-04T08:54:47Z

@willy-b I have been trying CSS solution too but couldn't find the solution. I thnk it doesn't solve the issue found by @bsclifton here above: #4820 (comment)

bsclifton · 2016-12-06T17:32:29Z

OK awesome; @luixxiul's PR has been accepted. @willy-b, can you rebase?

I think you're safe to own this change (the darkening of the modal issue) since you had already started this with a PR. Your last commit that I tested was working great, but we just needed to tune the behavior with regards to the buy widget... choosing either to:

Keep the overlay and let it darken extra dark (and possibly creating an issue to track fixing that)
Keeping the overlay but then putting a style over the iframe body style, making background transparent (for the buy widget)

@bsclifton

- tried several CSS-only solutions for ensuring only the top-most modal had a gray background, but I think using some JS is actually simpler, so that is what I did here - keeping modalOverlay background when opening iframe w/ Coinbase widget (decision made w/ @bsclifton after trying alternatives) fixes #4808

willy-b · 2016-12-07T01:01:17Z

Hey @bsclifton, thanks again for reviewing. I pushed a commit implementing the first option (keeping the overlay for the Coinbase iframe) 4 days ago and force-pushed a rebase of that here just now.

(in case anyone wants to check something, I kept the old/pre-rebase branch at https://github.com/willy-b/browser-laptop/tree/issue-4808-old)

bsclifton · 2016-12-07T06:34:40Z

Great job! This looks and feels great 😄

@cezaraugusto, wanna give it a go?

cezaraugusto · 2016-12-07T14:03:16Z

++!! looks awesome, great work!

luixxiul · 2016-12-08T14:23:57Z

@willy-b Hi Willy, is it possible to add .last unless the modal contains .coinbaseOverlay? The iframe inside .coinbaseOverlay itself has the dark background.

bsclifton · 2016-12-08T16:52:07Z

@luixxiul would you be able to open a separate issue for that? I think it's possible, but would be good to track separately 😄

luixxiul added the design A design change, especially one which needs input from the design team. label Oct 27, 2016

bsclifton added this to the 0.13.1 milestone Dec 2, 2016

luixxiul mentioned this pull request Dec 4, 2016

Refactor about:preferences#payments #6009

Merged

4 tasks

bsclifton assigned cezaraugusto and bsclifton Dec 6, 2016

cezaraugusto merged commit 2728303 into brave:master Dec 7, 2016

luixxiul modified the milestones: 0.13.0, 0.13.1 Dec 7, 2016

luixxiul mentioned this pull request Dec 8, 2016

Remove a dark background to the bottom layer if the modal contains .coinbaseOverlay #6093

Closed

This was referenced Dec 21, 2016

Manual tests for Windows ia-32 0.13.0 Preview 6 #6335

Closed

Manual tests for Linux Mint 0.13.0 Preview 6 #6337

Closed

Manual tests for Windows x64 0.13.0 Preview 6 #6334

Closed

Manual tests for OS X 0.13.0 Preview 6 #6332

Closed

luixxiul mentioned this pull request Jan 16, 2017

background color of modal window on ledger advanced settings gets darker by clicking backup wallet button #4808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When Modal Overlays are layered, only bottom layer has a dark background (fix #4808) #4820

When Modal Overlays are layered, only bottom layer has a dark background (fix #4808) #4820

willy-b commented Oct 15, 2016

bbondy commented Oct 18, 2016

willy-b commented Oct 26, 2016

luixxiul commented Nov 23, 2016

willy-b commented Nov 23, 2016

bsclifton commented Nov 26, 2016 •

edited

Loading

willy-b commented Nov 27, 2016

willy-b commented Nov 28, 2016

luixxiul commented Nov 28, 2016

bsclifton commented Nov 28, 2016

willy-b commented Nov 29, 2016 •

edited

Loading

bsclifton commented Dec 2, 2016

willy-b commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

luixxiul commented Dec 4, 2016

willy-b commented Dec 4, 2016

willy-b commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

bsclifton commented Dec 4, 2016

luixxiul commented Dec 4, 2016

bsclifton commented Dec 6, 2016 •

edited

Loading

willy-b commented Dec 7, 2016

bsclifton commented Dec 7, 2016 •

edited

Loading

cezaraugusto commented Dec 7, 2016

luixxiul commented Dec 8, 2016

bsclifton commented Dec 8, 2016

When Modal Overlays are layered, only bottom layer has a dark background (fix #4808) #4820

When Modal Overlays are layered, only bottom layer has a dark background (fix #4808) #4820

Conversation

willy-b commented Oct 15, 2016

bbondy commented Oct 18, 2016

willy-b commented Oct 26, 2016

luixxiul commented Nov 23, 2016

willy-b commented Nov 23, 2016

bsclifton commented Nov 26, 2016 • edited Loading

willy-b commented Nov 27, 2016

willy-b commented Nov 28, 2016

luixxiul commented Nov 28, 2016

bsclifton commented Nov 28, 2016

willy-b commented Nov 29, 2016 • edited Loading

bsclifton commented Dec 2, 2016

willy-b commented Dec 4, 2016 • edited Loading

willy-b commented Dec 4, 2016 • edited Loading

luixxiul commented Dec 4, 2016

willy-b commented Dec 4, 2016

willy-b commented Dec 4, 2016 • edited Loading

willy-b commented Dec 4, 2016 • edited Loading

bsclifton commented Dec 4, 2016

luixxiul commented Dec 4, 2016

bsclifton commented Dec 6, 2016 • edited Loading

willy-b commented Dec 7, 2016

bsclifton commented Dec 7, 2016 • edited Loading

cezaraugusto commented Dec 7, 2016

luixxiul commented Dec 8, 2016

bsclifton commented Dec 8, 2016

bsclifton commented Nov 26, 2016 •

edited

Loading

willy-b commented Nov 29, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

willy-b commented Dec 4, 2016 •

edited

Loading

bsclifton commented Dec 6, 2016 •

edited

Loading

bsclifton commented Dec 7, 2016 •

edited

Loading