838784 - Add a transition to the sidebar-box

Status: NEW

(Firefox :: General, defect)

Description

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

Attachment: Patch

On Windows, using ctrl+i toggles the bookmarks sidebar. This toggling should be animated.

Using -100% for the margin-left doesn't work. I couldn't get any percent-values to work for margin-left on the sidebar box, but I could get non-relative units to work easily.

To work around this, I set a negative margin-left of 28em and a max-width of 28em. This should prevent sidebars that were too large from sticking out in the browser when they are supposed to be collapsed. 28em seemed to be pretty close to what the limit is currently, although I didn't see where that is set in our source code.

Comment 1

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] from comment #0)
> To work around this, I set a negative margin-left of 28em and a max-width of
> 28em. This should prevent sidebars that were too large from sticking out in
> the browser when they are supposed to be collapsed. 28em seemed to be pretty
> close to what the limit is currently, although I didn't see where that is
> set in our source code.

This style attribute seems to do it: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul?rev=2f54529528a9#1100

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 710898 [details] [diff] [review]
Patch

>+#sidebar-box[hidden] {
>+  display: -moz-box;
>+  visibility: visible;

visibility:visible is the default state.

>+#sidebar-box[hidden]:-moz-locale-dir(ltr) {
>+  margin-left: -28em;
>+}

This moves the content area off-screen when sidebar-box is narrower than 28em.

Comment 3

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

Attachment: Patch v2

This should fix the bug about shifting the browser.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 711012 [details] [diff] [review]
Patch v2

The negative margin lets the sidebar overlap adjacent elements such as #browser-border-start. You can see this on Windows in restored windows.

The transition is pretty bumpy over here and IMHO not good enough. We should be careful with any transition resizing the content area, as they are very expensive and therefore never really smooth.

Comment 5

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

Attachment: Patch v2 (rebased)

Rebased v2 patch. I'd still like to see us do something here. I'm pretty happy with the animation and I don't see enough jank here (tested on cnn.com) to warrant blocking this.

The only thing holding up a review request from my end is finding a fix for the #browser-border-start issue.

Comment 6

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

Attachment: WIP Patch

Dao, what do you think about this?

The transition from showing the sidebar to hiding it isn't the best because you can see the window background before the content stretches to cover it.

One idea I have is to do a transform:translateX(-sidebarWidth) on #content and then in a transitionend listener remove the transform. This would happen pretty close to the same time as the sidebar switching to visibility:collapse, so the #content would then expand to use the full width of the browser window.

Comment 7

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

Comment on attachment 8391817 [details] [diff] [review]
WIP Patch

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

::: browser/themes/windows/browser.css
@@ +1677,5 @@
>  #sidebar {
>    background-color: Window;
>  }
>  
> +#sidebar-box,

Remind me again - why is this only Windows?

Comment 8

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

(In reply to Mike Conley (:mconley) from comment #7)
> Comment on attachment 8391817 [details] [diff] [review]
> WIP Patch
> 
> Review of attachment 8391817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +1677,5 @@
> >  #sidebar {
> >    background-color: Window;
> >  }
> >  
> > +#sidebar-box,
> 
> Remind me again - why is this only Windows?

Because it was a WIP :)

Comment 9

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

Comment on attachment 8391817 [details] [diff] [review]
WIP Patch

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

So, tentative f+ - this looks sane. Can you post a more up-to-date patch?

Comment 10

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

Attachment: Patch

What do you think of this patch? The background behind #appcontent is briefly shown during the transition. I think it would be better if:
- when opening: #appcontent stayed the same size until the transition was complete, at which it then would shrink
- when closing: #appcontent expands, and then the sidebar shrinks

Comment 11

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

Jared, please provide a point estimate.
Marco, please add this bug to the current iteration.

Comment 12

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 13

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

Dao, review ping?

Comment 14

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

jaws:

Hrm - this looks kinda weird to me, especially on Windows Aero Glass. That flash of background during the transition just looks kinda broken.

We might want to run this by some UX folks to get their input - maybe they've got some ideas on how we can minimize the issue.

Functions just fine otherwise.

Comment 15

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

(In reply to Mike Conley (:mconley) from comment #14)
> jaws:
> 
> Hrm - this looks kinda weird to me, especially on Windows Aero Glass. That
> flash of background during the transition just looks kinda broken.
> 
> We might want to run this by some UX folks to get their input - maybe
> they've got some ideas on how we can minimize the issue.
> 
> Functions just fine otherwise.

Yeah, this is what I meant by:

"What do you think of this patch? The background behind #appcontent is briefly shown during the transition. I think it would be better if:
- when opening: #appcontent stayed the same size until the transition was complete, at which it then would shrink
- when closing: #appcontent expands, and then the sidebar shrinks"

Comment 16

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

I just can't figure out how to do that --^ :)

Comment 17

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

Sounds like you need to take the sidebar box out of the flow of the document when it's transitioning. position: absolute inside a thin parent that has position: relative?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8424900 [details] [diff] [review]
Patch

Sounds like this patch isn't ready for prime time or review.

Comment 19

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

Comment on attachment 8424900 [details] [diff] [review]
Patch

Also cancelling review until I hear more about whether or not the position: absolute / position: relative thing worked out.

Comment 20

[Dão Gottwald [:dao]]

Even if your suggestion doesn't work, I don't think this would make this patch acceptable. An irritatingly weird transition seems worse than having no transition.

Comment 21

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

I'm not sure what our options are here, then. Do we know what exactly UX had in mind for this?

Comment 22

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I am not aware of a UX decision about this.
Generally, I agree with Dão that no transition is better than a choppy one (but I haven't tried the patch yet).
Should there be a UX bug about this? Perhaps there is a different kind of transition we could use that isn't so expensive…

Comment 23

[Dão Gottwald [:dao]]

I think our best bet is to try the suggestion from comment 17 and get ui-review on that. (I suspect the delayed content area shrinking might still feel weird.) Otherwise I'm thinking wontfix for lack of good alternatives, unless someone has a totally new idea. The history and bookmark sidebars aren't high-profile features and to me at least the instant opening doesn't feel particularly jarring or unpolished in this case.

Comment 24

[Marco Mucci [:MarcoM]]

Removed for Iteration 32.3 for GMC to discuss.