1196247 - Findbar is hidden only after entering DOM fullscreen and that ruins the whole thing

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[arni2033]

Attachment: Findbar is hidden only after entering DOM fullscreen.webm

STR:   (Win7, Nightly 43.0a1 (2015-08-18))
1. Open any video, e.g. https://www.youtube.com/watch?v=zMc25iLrpE8
2. Open findbar (Ctrl+F or F3)
3. Click fullscreen button on the video

Result:       DOM transition was ended, and only after that findbar collapsed
Expectations: Findbar should collapse while the screen is still black

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

I believe it is because the find bar has its own fancy transition...

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

FWIW, hiding findbar when entering DOM fullscreen was initially implemented in bug 701251.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8650279 [details] [diff] [review]
patch

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

::: browser/base/content/tabbrowser.xml
@@ +199,5 @@
>            return findBar;
>          ]]></body>
>        </method>
>  
> +      <method name="removeFindBar">

Hmm, I'm not so sure about this. Nothing else is using destroy(), and it looks intended to be more of a finalizer than another arbitrary way to close the find bar.

If this is just the 150ms hiding transition bleeding over into fullscreen mode, that's what we should fix directly.

First thought that comes to mind, because I vaguely remember using it elsewhere, is to add an "immediate" arg to close(). Then it can set an attribute to temporarily suppress the transition and clear it afterwards. [Oh, looks like I'm thinking of startFade() in videocontrols.xml]

A pure CSS solution like |findbar:fullscreen { transition: none; }| would be nice, but actually seems tricky... It's not enough to simply disable the transition in full-screen mode, because the bar should still transition if the user is opening/closing it when they're already in FS mode... It's only the special case of closing it as we're entering FS mode that's an issue.

Comment 5

[arni2033]

BTW, do you think that:
1) Findbar should NOT restore it's state after exiting DOM fullscreen?
2) Notifications like "Nightly prevented this site from opening a pop-up window" should NOT hide when you enter DOM fullscreen?

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Justin Dolske [:Dolske] from comment #4)
> Comment on attachment 8650279 [details] [diff] [review]
> patch
> 
> Review of attachment 8650279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabbrowser.xml
> @@ +199,5 @@
> >            return findBar;
> >          ]]></body>
> >        </method>
> >  
> > +      <method name="removeFindBar">
> 
> Hmm, I'm not so sure about this. Nothing else is using destroy(), and it
> looks intended to be more of a finalizer than another arbitrary way to close
> the find bar.

Well, yes, it seems to be a finalizer, but given I'm immediately remove that element from the document, I think we should call the finalizer there, especially given the comment above findbar.xml:destroy.

Probably we can cc someone who is more familiar with findbar to judge whether it is a good idea?

> If this is just the 150ms hiding transition bleeding over into fullscreen
> mode, that's what we should fix directly.
> 
> First thought that comes to mind, because I vaguely remember using it
> elsewhere, is to add an "immediate" arg to close(). Then it can set an
> attribute to temporarily suppress the transition and clear it afterwards.
> [Oh, looks like I'm thinking of startFade() in videocontrols.xml]

I thought about that way but I feel that is more complicated than just completely remove the findbar.

In addition, experience shows that, using visibility property to hide element instead of "display: none" has performance effect, because the hidden element and its descendants would still need reflow.

When I implement bug 1160023, where I initially used similar technique on a fixed-position element to hide it, I received a bunch of performance regression warnings on resize talos vary from 3%-9%.

For this reason, I also expect destroying the findbar here could potentially contribute to reducing the overall time for fullscreen change, though not significant.

> A pure CSS solution like |findbar:fullscreen { transition: none; }| would be
> nice, but actually seems tricky... It's not enough to simply disable the
> transition in full-screen mode, because the bar should still transition if
> the user is opening/closing it when they're already in FS mode... It's only
> the special case of closing it as we're entering FS mode that's an issue.

It should be |#main-window[inDOMFullscreen] findbar { ... }| if we want that way.

Comment 7

[Justin Dolske [:Dolske]]

cc'd Mike earlier, but let's NI for his opinion here. :)

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> Well, yes, it seems to be a finalizer, but given I'm immediately remove that
> element from the document, I think we should call the finalizer there,
> especially given the comment above findbar.xml:destroy.
> 
> Probably we can cc someone who is more familiar with findbar to judge
> whether it is a good idea?

I think that passing a parameter to `findbar.close` that specifies to skip the transition would be a better idea.
Something like `gFindbar.close(true)` and the 'close' method in findbar.xml would take a parameter 'aCancelAnim', which would:
1) Set an attribute called 'cancelanim' or 'cancelanimation' on the findbar, which would set `transition-duration: 0` and `transition-delay: 0` via a CSS selector in findBar.css IF `aCancelAnimation` is TRUE.
2) In a `requestAnimationFrame` call, remove the attribute again.

Without the transition, the findbar will hide instantaneously.

How does this sound?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> > Well, yes, it seems to be a finalizer, but given I'm immediately remove that
> > element from the document, I think we should call the finalizer there,
> > especially given the comment above findbar.xml:destroy.
> > 
> > Probably we can cc someone who is more familiar with findbar to judge
> > whether it is a good idea?
> 
> I think that passing a parameter to `findbar.close` that specifies to skip
> the transition would be a better idea.
> Something like `gFindbar.close(true)` and the 'close' method in findbar.xml
> would take a parameter 'aCancelAnim', which would:
> 1) Set an attribute called 'cancelanim' or 'cancelanimation' on the findbar,
> which would set `transition-duration: 0` and `transition-delay: 0` via a CSS
> selector in findBar.css IF `aCancelAnimation` is TRUE.
> 2) In a `requestAnimationFrame` call, remove the attribute again.

That would need nested requestAnimationFrame, otherwise it won't work because the first callback would run before the next style computation.

I'm interested in the reason why you don't like to destroy it completely.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> That would need nested requestAnimationFrame, otherwise it won't work
> because the first callback would run before the next style computation.

Ok, well if that's the case, we can also remove the attribute inside the `open` method (above the `close` method in findbar.xml).

> I'm interested in the reason why you don't like to destroy it completely.

Because I'm wary of the side-effects of the findbar behavior when the user presses CTRL/CMD+G after the findbar was destroyed. There is a certain amount of state - minimal, thank goodness - being stored during the lifetime of the findbar.
I think that inventing a mechanism to restore that would be more work than I proposed above.

Comment 11

[Mike de Boer [:mikedeboer]]

And sorry for the late reply, I'm getting back to full power after a few miserable weeks.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1196247 - Hide findbar without animation when entering DOM fullscreen.

Bug 1196247 - Hide findbar without animation when entering DOM fullscreen.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 8668267 [details]
MozReview Request: Bug 1196247 - Hide findbar without animation when entering DOM fullscreen.

I'm swamped, so moving this to Mike.

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8668267 [details]
MozReview Request: Bug 1196247 - Hide findbar without animation when entering DOM fullscreen.

https://reviewboard.mozilla.org/r/20949/#review19237

LGTM! Can you do a try-run before landing this? The biggest risk with findbar changes are regressions.

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dc1d62b8517

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/fx-team/rev/0885a18c2fc78f485f62e0fade5a56993b15c16d
Bug 1196247 - Hide findbar without animation when entering DOM fullscreen. r=mikedeboer

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0885a18c2fc7

Comment 18

[Fahmida Noor]

I have reproduced this bug with Firefox Nightly 43.0a1 (Build Id : 20150819030206) on Windows 7(64-bit) with the instructions from comment 0.

Verified as fixed with Latest Firefox Beta 44.0b7 (Build ID : 20160107144911)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20160108]