871158 - Windows: Missing borders around the toolbox and customize area in customization mode

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[Alejandro Rodriguez [:Alopepeo]]

Attachment: Missing borders when maximized

1. Maximize your Firefox window 
2. Enter customization mode

Result:

There are no borders at both sides of the nav-bar and at the bottom of the customization palette.

Expected result:

The appearance of the customization mode should be the same on maximized and restored windows.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I think the changes for bug 870574 might be useful for Windows and Linux to get the same shadow/border when in customization mode.

Comment 2

[:Gijs (he/him)]

(In reply to Alejandro Rodriguez [:Alopepeo] from comment #2)
> Created attachment 748433 [details]
> Missing borders when maximized
> 
> 1. Maximize your Firefox window 
> 2. Enter customization mode
> 
> Result:
> 
> There are no borders at both sides of the nav-bar and at the bottom of the
> customization palette.
> 
> Expected result:
> 
> The appearance of the customization mode should be the same on maximized and
> restored windows.

FWIW, as far as I can tell there are currently no borders in restored windows, either. :-\

Comment 3

[:Gijs (he/him)]

Attachment: WIP

This is where I'm at at the moment. It sort of works, but I'm having issues with the navbar's top border / tabbar's bottom border. It needs to have a radius together with the side borders of the navbar, but I can't seem to get this to work properly, and I also can't figure out where the bottom border on the tab bar is actually coming from and/or how I'd workaround the bad effects it's creating here.

Comment 4

[:Gijs (he/him)]

Attachment: Screenshot of what I'm seeing

Apologies for slight blurriness, screenshot was over VNC from a hidpi mac. Doesn't detract from the visibility of the issue here though.

So, Matt, I couldn't figure out where the tab's border comes from, on Windows. Any clues? :-)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 6

[:Gijs (he/him)]

Comment on attachment 764762 [details] [diff] [review]
WIP

Matt and I talked about this, it's a gradient. Clearing feedback request for now, and marking this obsolete as it won't do, obvs.

Comment 7

[:Gijs (he/him)]

Attachment: Patch v1

OK, this works well in Aero Basic, Aero Glass, and Win7 Classic. Testing XP in a bit.

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> Created attachment 773217 [details] [diff] [review]
> Patch v1
> 
> OK, this works well in Aero Basic, Aero Glass, and Win7 Classic. Testing XP
> in a bit.

XP Seems to look good with this as well (save other issues with how far the titlebar extends... but that has nothing to do with this bug).

Comment 9

[Dão Gottwald [:dao]]

Ugh. That code looks like it's guaranteed to break whenever we touch the border styling for when you're not customizing. Can we drop the odd grid around the window while customizing? It causes a whole class of bugs such as bug 885069, bug 883145, bug 885066 / bug 870602 and it seems questionable whether it improved usability even if it worked as intended (e.g. it reduces the space available in toolbars, which isn't at all what you want while customizing).

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 773217 [details] [diff] [review]
Patch v1

As I've communicated in-person to multiple people, I agree with Dão that this custom window padding for customization mode seemingly adds more costs in maintainability/complexity than the benefit it's supposed to achieve. There is also the performance of the animation (bug 873060) which still needs more work otherwise the browser feels janky upon entering/exiting customization mode (I'm testing on an XP VM with the latest UX Nightly).

I discussed this with shorlander who was fine to get rid of it but he deferred me to Madhava who wants to keep it if possible.  We discussed the benefits it's intended to achieve but I think I'm forgetting some (there was some blurriness between some IIRC). From memory here are some 
1) A clear indication that the user is in a special mode to help explain why things work differently.
2) It's somewhat whimsical with the animation and something you can show off to friends. This gives some kind of attachment and can be somewhat viral.

This patch is too fragile as Dão says. Have you looked into changing the rules/rulesets used for adding the borders in the first place which would reduce duplication? e.g. add a rule to some rulesets that cause the border to apply in customization mode when it wouldn't normally. Let me know if this isn't any cleaner.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

This also affects at least OS X as well.

Comment 13

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #12)
> This also affects at least OS X as well.

This confuses me. The OS X bits were implemented in bug 870574. Do we know when they regressed?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

I think I'm totally wrong on this bug - I thought this bug was reporting missing dotted outlines around customization areas, but a re-read makes it clear that it's about borders around the toolbar / customization elements themselves.

So please ignore comment 12 for now. :)

Comment 16

[u428464]

(In reply to Mike Conley (:mconley) from comment #14)
> I think I'm totally wrong on this bug - I thought this bug was reporting
> missing dotted outlines around customization areas, but a re-read makes it
> clear that it's about borders around the toolbar / customization elements
> themselves.
> 
> So please ignore comment 12 for now. :)

Is about the borders (https://bug871158.bugzilla.mozilla.org/attachment.cgi?id=748433). It's in maximized mode only.

Comment 17

[Justin Dolske [:Dolske]]

philipp says this is highly visible as a problem on Windows when full-screen.

Comment 18

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #17)
> philipp says this is highly visible as a problem on Windows when full-screen.

Can we add this to the backlog for the upcoming iteration?

Unassigning because I'm not currently working on this.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

See bug 941244 comment 9 - same applies here.

Comment 20

[Justin Dolske [:Dolske]]

checked in a current nightly on phlsa's VM, and this appears to have been fixed at some point. (Comparing to screenshot in comment 16)