Open Bug 939480 Opened 6 years ago Updated 2 years ago

Historyswipe bounce behavior should use css TranslateY instead of screenshot

Categories

(Core :: Widget: Cocoa, defect, P2)

x86
macOS
defect

Tracking

()

People

(Reporter: BenWa, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: tpi:-)

Attachments

(1 obsolete file)

Currently the bounce behavior appears to be taking a screenshot of the page. This has several problems:
* It requires two needless repaint. (1) Taking the screenshot, (2) Repainting when re-activating. This is bad for performance because it causes a stutter when over scrolling which is very noticeable. This is much worse on retina.
* Animation on the page are stopped because we are displaying a screen.
* I imagine things like hit testings are either not done or not done properly.

We already have 'active' layers for the browser element which means that we can transform the element *very* efficiently (without re-rasterizing) and continue to update in real time. Fixing this will give us better performance, stop that stutter at the start/end of the animation, keep animations playing, and solve the low res hidpi screenshot (even if a high hidpi screenshot would too).
I did a quick test and the following works fine, we just need to animate it properly:
gBrowser.style.transform = "translateY(50px)";
(In reply to Benoit Girard (:BenWa) from comment #0)
> This has several problems:

Increased memory usage.
Coincidentally, I've just spoken with Felipe about this. We just need to make sure to show the proper background behind the 'page'. Here's Felipe's detailed write-up:

Here's an idea for what we talked about yesterday on being able to move the browser element on overscroll instead of taking a snapshot of it.

Here's the current DOM structure, as far as I can tell (using DOM inspector to see it):

<vbox class="browserContainer">
  <stack class="browserStack">
    <browser>
    <stack id="historySwipeAnimationContainer">
      <box id="historySwipeAnimationPreviousPage">
      <box id="historySwipeAnimationCurrentPage">
      <box id="historySwipeAnimationNextPage">
    </stack>
  </stack>
</vbox>

I think we could use either the browserStack or browserContainer to host the gray background image, and we wouldn't need to even create the other stack and boxes for overscroll: we can just apply the box-shadow and the transform:translate directly to <browser> or the browserStack.

One thing to be mindful about is that I found out that the developer tools "responsive mode" also modify the DOM around this container/stack.. And they also have their background-image and box-shadow. See
http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/responsivedesign.inc.css#9 and
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/responsivedesign.jsm#365
I *think* implementing this change will also move us in the right direction to have overscroll handled by the APZC once it's ready.
Duplicate of this bug: 868166
Blocks: 946469
Attached patch Part 1: Backout bug 946862 (obsolete) — Splinter Review
Backout 1c645ff95f2c (bug 946862) to reenable vertical overscroll. Marking this r+ since this is a straight backout.
Attachment #8343748 - Flags: review+
(In reply to Stephen Pohl [:spohl] from comment #6)
> Created attachment 8343748 [details] [diff] [review]
> Part 1: Backout bug 946862
> 
> Backout 1c645ff95f2c (bug 946862) to reenable vertical overscroll. Marking
> this r+ since this is a straight backout.

I don't think this is the right place to re-enable this feature without implementing what is described in this bug. Instead you should clone bug 946862 and keep the original people on CC. Once a new bug is created for re-enabling then you should state why you re-enabling this feature since arguments for keeping this feature disabled were provided for bug 946862 that were agreed open by landing the patch to disable that feature.
(In reply to Benoit Girard (:BenWa) from comment #7)
> I don't think this is the right place to re-enable this feature without
> implementing what is described in this bug.

I may be misunderstanding, but part 1 isn't meant to land without a part 2 that actually fixes this bug.

> Instead you should clone bug
> 946862 and keep the original people on CC. Once a new bug is created for
> re-enabling then you should state why you re-enabling this feature since
> arguments for keeping this feature disabled were provided for bug 946862
> that were agreed open by landing the patch to disable that feature.

The bug title for bug 946862 was "Disable overscroll on OS X in Nightly until it's implemented like bug 939480 suggests". Are there other arguments for keeping vertical overscroll disabled beyond this bug? The other comments seemed to refer to horizontal overscroll, which is handled separately in bug 860493 and dependent bugs.
(In reply to Stephen Pohl [:spohl] from comment #8)
> I may be misunderstanding, but part 1 isn't meant to land without a part 2
> that actually fixes this bug.
> 

Sorry! Just a misunderstanding on my part. Please carry on :)
Depends on: 942558
Does anyone have a timeframe when this will be worked on? The bounce behaviour is very much appreciated for platform consistency.

(Also trying to put this on fx-backlog, though I'm not sure if this bug qualifies …)
Flags: firefox-backlog?
(In reply to Florian Bender from comment #10)
> Does anyone have a timeframe when this will be worked on? The bounce
> behaviour is very much appreciated for platform consistency.
> 
> (Also trying to put this on fx-backlog, though I'm not sure if this bug
> qualifies …)

I don't think this is something that the desktop frontend team is in the best position to fix, but Felipe might be able to clarify.
Flags: needinfo?(felipc)
(In reply to :Gijs Kruitbosch from comment #11)
> I don't think this is something that the desktop frontend team is in the
> best position to fix, but Felipe might be able to clarify.

Why not? translateY is a CSS properly that is well implemented and optimized by the platform. It is used extensively by gaia.
Comment on attachment 8343748 [details] [diff] [review]
Part 1: Backout bug 946862

I think we should no longer revert to this behavior. The vertical bounce effect and the history swipe animations should be separated, with the ability to turn either one on/off without affecting the other.
Attachment #8343748 - Attachment is obsolete: true
(In reply to Benoit Girard (:BenWa) from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > I don't think this is something that the desktop frontend team is in the
> > best position to fix, but Felipe might be able to clarify.
> 
> Why not? translateY is a CSS properly that is well implemented and optimized
> by the platform. It is used extensively by gaia.

Because it's the hooking into the platform bits as to when overscroll/bounce-back is used that would need changing as well, and that's fundamentally the docshell and core's business (related: bug 1050804). Plus, just using translateY wouldn't address the scrollbar size changing the way it does in other parts of OS X. Finally, we'd need to fix it in all frontend <browser/frame> implementations separately (e.g. sidebars, which I suspect translating gBrowser might not work correctly for, plus view source, plus iframes, plus the separate iframes in SDK widget panels, ... )
Flags: firefox-backlog?
Flags: needinfo?(felipc)
Summary: Bounce behavior should use css TranslateY instead of screenshot → Historyswipe bounce behavior should use css TranslateY instead of screenshot
Whiteboard: tpi:-
Blocks: 1382560
No longer blocks: 1382560
No longer blocks: history-swipe
Priority: -- → P2
Depends on: 946563
You need to log in before you can comment on or make changes to this bug.