Closed Bug 893446 Opened 11 years ago Closed 11 years ago

Page jumps when showing/hiding Find In Page toolbar

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox25 + verified
firefox26 --- verified

People

(Reporter: Dolske, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

sample
499 bytes, text/html
Details
potential covering content that can be scrolled back
60.86 KB, image/png
Details
video of automatically adjusting scroll
423.76 KB, video/webm
Details
v4
14.19 KB, patch
dao
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
screenshot.png
291.23 KB, image/png
Details 
Bug 869543 moves the Find In Page toolbar to the top of the window. Some have remarked that this can be a bit jarring, as now it caused a large section of the window to shift up/down. (As does showing/hiding the bookmarks toolbar, or showing/hiding a notification bar.)

Bug 893013 comment 1 suggests one solution:

---
A better solution to this problem is simply not to shift the page at all. Instead, the scroll position is changed slightly so that the page appears to stand still as the find bar comes down. For most pages, this should work flawlessly. The only issue I can think of is objects that are fixed to the top of the viewport. This is much better solution than to add a preference and ask users to decide which solution they prefer.

Safari has done this for years, by the way: http://www.youtube.com/watch?v=RnnDk8yBGT0

Bug 248715 is about is specifically about this issue, applied to toolbars in general.
---

I remember faaborg making a similar suggestion in the past.

We should decide if the current UX is bad enough to require a fix/backout, or if this is acceptable as-is. (I'm just filing the bug, haven't really thought about it.)
(In reply to Justin Dolske [:Dolske] from comment #0)
> Bug 869543 moves the Find In Page toolbar to the top of the window. Some
> have remarked that this can be a bit jarring, as now it caused a large
> section of the window to shift up/down. (As does showing/hiding the
> bookmarks toolbar, or showing/hiding a notification bar.)
> 
> Bug 893013 comment 1 suggests one solution:
> 
> ---
> A better solution to this problem is simply not to shift the page at all.
> Instead, the scroll position is changed slightly so that the page appears to
> stand still as the find bar comes down. For most pages, this should work
> flawlessly. The only issue I can think of is objects that are fixed to the
> top of the viewport. This is much better solution than to add a preference
> and ask users to decide which solution they prefer.
> 
> Safari has done this for years, by the way:
> http://www.youtube.com/watch?v=RnnDk8yBGT0
> 
> Bug 248715 is about is specifically about this issue, applied to toolbars in
> general.
> ---
> 
> I remember faaborg making a similar suggestion in the past.
> 
> We should decide if the current UX is bad enough to require a fix/backout,
> or if this is acceptable as-is. (I'm just filing the bug, haven't really
> thought about it.)


Safari and Chrome's solution is completely bad.
FindBar should not obscure any item of contents.
Because the important item including an account info and a search box and menus is often put in the upper part of the page.
(In reply to Alice0775 White from comment #1)
> Safari and Chrome's solution is completely bad.
> FindBar should not obscure any item of contents.
> Because the important item including an account info and a search box and
> menus is often put in the upper part of the page.

The implementation could just change the scroll-position on the fly, so the content behind the toolbar could be easily scrolled into view again.
This is why developers decided the findbar aligned bottom at Firefox 1.0. Why do the guys fixing bug 869543 ignored this important point? I think that we should back it out until UX team find a good solution of this point.
(In reply to Peter Retzer (:pretzer) from comment #2)
> The implementation could just change the scroll-position on the fly, so the
> content behind the toolbar could be easily scrolled into view again.

Indeed, that is exactly what I was suggesting, and is what Safari does. It is not like Chrome’s implementation, where content at the top right is impossible to see without closing the find bar. Let me also point out that the stuff at the very top of the page is unlikely to be very relevant when someone is using the find bar; they are more likely looking for something further down the page, where it is not easy to find. Therefore, scrolling the top of the page slightly out of view is unlikely to cause any big problems.
(In reply to David Regev from comment #4)
> (In reply to Peter Retzer (:pretzer) from comment #2)
> > The implementation could just change the scroll-position on the fly, so the
> > content behind the toolbar could be easily scrolled into view again.
> 
> Indeed, that is exactly what I was suggesting, and is what Safari does. It
> is not like Chrome’s implementation, where content at the top right is
> impossible to see without closing the find bar. Let me also point out that
> the stuff at the very top of the page is unlikely to be very relevant when
> someone is using the find bar; they are more likely looking for something
> further down the page, where it is not easy to find. Therefore, scrolling
> the top of the page slightly out of view is unlikely to cause any big
> problems.

I think Safari's idea does not solve this problem.
I do not have MAC, please try the following case

Case#1
1. Open Findbar. Then, page scrolled a bit --- (OK, Safari's behavior)
2. Scroll to top in order to view covered item of contents.
3. Close Findbar -- The page jumps up.  <-- this is problem

Case#2
1. Open Findbar
2. If page does not have vertical scrollbar
   --- The page jumps down  <-- this is problem
3. Close  Findbar
   --- The page jumps up  <-- this is problem

Case#3
1. Open Findbar
2. Navigate a link in a same tab
   --- The Findbar will close  <--  I think this is problem
   Even if the Findbar will not close,
   --- Top of page is covered <--  this is problem
My conclusion, The page content SHOULD NOT be hidden by Findbar
(In reply to Alice0775 White from comment #5)
> I think Safari's idea does not solve this problem.
> I do not have MAC, please try the following case
> 
> Case#1
> 1. Open Findbar. Then, page scrolled a bit --- (OK, Safari's behavior)
> 2. Scroll to top in order to view covered item of contents.
> 3. Close Findbar -- The page jumps up.  <-- this is problem
> 
> Case#2
> 1. Open Findbar
> 2. If page does not have vertical scrollbar
>    --- The page jumps down  <-- this is problem
> 3. Close  Findbar
>    --- The page jumps up  <-- this is problem
> 
> Case#3
> 1. Open Findbar
> 2. Navigate a link in a same tab
>    --- The Findbar will close  <--  I think this is problem
>    Even if the Findbar will not close,
>    --- Top of page is covered <--  this is problem


In response to comment 5, in my opinion:

Case #1: Not a problem, by default you can only close the findbar by clicking on the X or by going to the menu entry. To do these things you need to move your sight away from the content (unless you're really good with the mouse or you'd rather press at least a combination of 4 of 5 sequential keys in your keyboard), so the fact that the content shifts in this case would be hardly noticeable. Even when you are mentally saving the position of something in the page, the shift won't be that big that you won't be able to re-adjust yourself quickly I think. (Of course, I can only speak for myself here).

Case #2: Hardly a problem either, if it doesn't have scrollbars then they won't have that much content to begin with. If it moves slightly it'll be even easier for your eyes to catch up.

Case #3: The find bar doesn't close for me when I navigate to a link in the same tab...

I'd like to present another case of my own though.

Case #4: Framesets. Even though in disuse and completely obsolete, they do still exist. These would always be shifted as you can't scroll a frameset (well, not a "properly" built one anyway), only the frames within it. I don't know if/how Safari handles this case.

While developing FindBar Tweak, my first approach to move the find bar to the top was a similar implementation of what is currently being done, just physically move it to the top. However I decided against it and the main reason was precisely the page shifting that occurred. I've tried it at least twice since then and always came to the same conclusion on this. Currently, the add-on can do this through a hidden setting but I chose not to make it a full "public" feature precisely for the reason that it shifts the content.

I'll give you a point on this though, the open and close animation makes it more tolerable, where it makes it seem like the content moves rather than just instantly shift. However, as in bug 893016, the animation isn't always done, and on cases where Firefox may lag (for whatever reason, even because some other process is hogging cpu) this animation seems like it's skipped altogether. So I wouldn't rely on this point to justify the move to the top either.

Again, in FindBar Tweak, the only method I've chosen to move the find bar to the top is through a Google Chrome-like findbar. Smaller and thinner, it won't cover much content that way. I would not choose this for the default method however, as making the full width find bar on top, like it is now, simply cover the top part would be to hide too much content and I agree with everyone else on this: it's a bad idea if you can't scroll back to it. It would probably be acceptable only for the quickfind bar, and even that I'm not sure of.

Personally, I think the best way overall would really be to back out of moving the find bar to the top, it may not fit as well with the desired new Firefox look, but it is the most certain not to cause any of the issues raised.

Scrolling the content as the find bar animates, so it seems like it doesn't move at all, is also an acceptable option in my opinion. If you think about it, this is pretty much the old behavior when find bar was on the bottom, except now it's done on the top side. That behavior still suffered from case #1 in comment 5 (except, once again, from the bottom and not from the top), the only differences would be case #2 in comment 5 and case #4 that I presented here. There will always be content shifting with this method in some specific cases, there's no avoiding that.

Just my 2 cents.
why don't you use position: fixed
adding this rule solved most of the problem:
.tabbrowser-tabbox findbar {
  position: fixed;
}

if you want the findbar to stretch just set the width each time you show the findbar on a tab
So, No solution of page jump.
Findbar on the top is problematic even if safari/chrome is doing
(In reply to Alice0775 White from comment #5)
> Case#1
> 1. Open Findbar. Then, page scrolled a bit --- (OK, Safari's behavior)
> 2. Scroll to top in order to view covered item of contents.
> 3. Close Findbar -- The page jumps up.  <-- this is problem

This is a problem only in this particular specific case. But how often would this even happen? If you’re finding some text, why are you scrolling up all the way while doing that? Also, the same exact thing happens with the find bar at the bottom: scroll to the very bottom (which should actually happen often while finding), close the find bar, and the page shifts down. Similarly, with the find bar at the bottom, objects fixed to the bottom (such as chat windows in Gmail) jump a lot. In any case, these rare issues are nothing compared to the current behaviour, where the page jumps almost every single time.

> Case#2
> 1. Open Findbar
> 2. If page does not have vertical scrollbar
>    --- The page jumps down  <-- this is problem
> 3. Close  Findbar
>    --- The page jumps up  <-- this is problem

Such a page would move around whether the find bar is at the top or the bottom.

> Case#3
> 1. Open Findbar
> 2. Navigate a link in a same tab
>    --- The Findbar will close  <--  I think this is problem
>    Even if the Findbar will not close,
>    --- Top of page is covered <--  this is problem

The find bar should go away whenever you’re switching to a new page or tab.

Here’s a suggestion to avoid all these problems (though it may introduce some others): what if the find bar were merged with the navigation bar, such that the location and search bars would turn into the find bar when conducting a find?
Attached file sample — 
page will jump when open Findbar.
So, Safari's FindBar does not solve this problem
Assignee: nobody → alice0775
(In reply to Justin Dolske [:Dolske] from comment #0)
> Bug 869543 moves the Find In Page toolbar to the top of the window. Some
> have remarked that this can be a bit jarring, as now it caused a large
> section of the window to shift up/down. (As does showing/hiding the
> bookmarks toolbar, or showing/hiding a notification bar.)

If I recall correctly, this was sometimes considered a feature for notification bars as a means to draw attention to them. I imagine it's similarly helpful when using the find bar for the first time. I'm not yet convinced it's a real annoyance or inconvenience when using the find bar regularly, although I see how it can be irritating if you're used to how the find bar at the bottom behaved.

(In reply to Alice0775 White from comment #6)
> My conclusion, The page content SHOULD NOT be hidden by Findbar

Whether the find bar is at the bottom or at the top, whether we scroll or don't scroll, there's no reasonable way to prevent the find bar from cutting away some part of the page. That's not really a problem since find results are scrolled into view, as already mentioned in comment 2.
Assignee: alice0775 → nobody
(In reply to Dão Gottwald [:dao] from comment #14)
> I'm not yet
> convinced it's a real annoyance or inconvenience when using the find bar
> regularly
The page shifting is preventing from me using Nightly for even short periods of time because I get motion sickness. The page often shifts when I'm actively focused on some text:

- starting to find existing text when I'm focused looking at a word to type it into the find bar
- closing the find bar after finding the text when I'm focused on reading the link text and clicking it
- just opening and closing the find bar in general when trying to browse (as opposed to what I need to do of closing my eyes when hitting cmd-f and escape)

I believe I run into these situations even more because I have "Search for text when I start typing" selected. I often navigate by just typing, so if I'm looking at a page with many links, e.g., message board/forum, I just type some word in the link to move focus there to then open in a background tab with cmd-enter which dismisses the typeahead find. This causes the page to shift down and up in less than a second, and this happens easily up to 10 times in 10 seconds.

At that point, I'm starting to get dizzy and need to stop using Firefox for a few minutes.
(In reply to comment #14)
> (In reply to Justin Dolske [:Dolske] from comment #0)
> > Bug 869543 moves the Find In Page toolbar to the top of the window. Some
> > have remarked that this can be a bit jarring, as now it caused a large
> > section of the window to shift up/down. (As does showing/hiding the
> > bookmarks toolbar, or showing/hiding a notification bar.)
> 
> If I recall correctly, this was sometimes considered a feature for notification
> bars as a means to draw attention to them. I imagine it's similarly helpful
> when using the find bar for the first time. I'm not yet convinced it's a real
> annoyance or inconvenience when using the find bar regularly, although I see
> how it can be irritating if you're used to how the find bar at the bottom
> behaved.

This analogy doesn't really hold here.  Notification bars are usually not displayed as a direct result of a user command, contrary to the find bar.  The thing that is realy anoying about the find bar shifting the whole page down is that when you press Cmd+F, you're usually focusing your eyes somewhere on the page, and then things will suddenly move which ruins your focus even if you don't get motion sickness because of it.  This behavior is terrible regardless of how the old find bar used to work.
Previously we've had to go out of our way to draw attention to the Find bar to users who don't know about it (flashing it). Flashing an element to draw attention to it really needs to be a last resort because it literally screams that we couldn't find a more natural way to introduce it to the user.

It is important to remember that users who "do type-ahead-find 10 times in 10 seconds" (aka Wizards) are a super tiny minority of our users.
(In reply to Jared Wein [:jaws] from comment #17)
> screams that we couldn't find a more natural way to introduce it to the user.
Just to be clear, animating 90%+ of the screen is a more natural way to introduce the find bar on every show/hide? Were there any indications that Busy Bees/Enthusiasts found moving their content around was the best solution?
(In reply to Jared Wein [:jaws] from comment #17)
> Previously we've had to go out of our way to draw attention to the Find bar
> to users who don't know about it (flashing it). Flashing an element to draw
> attention to it really needs to be a last resort because it literally
> screams that we couldn't find a more natural way to introduce it to the user.
> 
> It is important to remember that users who "do type-ahead-find 10 times in
> 10 seconds" (aka Wizards) are a super tiny minority of our users.

I don't think this is a valid argument. Flashing the find bar the first time it is shown is a good way to introduce it to the user (keyword: introduce). Even non-experienced users who already know about the find bar, who by the way are not the minority you consider them to be if you include casual users who just want to find a piece of text every once in a while, will find the page shifting behavior bothersome.

There is no natural way to introduce anything, that is the nature of introducing things, you need to go out of the normal way to feature it and make it more than obvious. In the case of the find bar, what you are saying is you need to go out of your way every time you open it to show every user where the find bar is, always. This type of "feature" would only be necessary once, ever.

I've already voiced my opinion on this subject, scrolling page content as the find bar opens so it doesn't shift content is acceptable, backing out of moving it to the top and place it on the bottom again would be preferred. But you are suggesting to consider the content shifting as an actual feature, and this I have to completely oppose. It would be extremely bad judgement and very poor strategy to consider this to be any kind of intentional feature.
I almost have a patch ready that makes sliding of the findbar AND the content area very smooth, which *should* satisfy your concerns, I think. It would also solve bug 893672.

The jagged animation is the cause of most concerns expressed here and in other bugs, I think (which shows how important they are to get right if you use 'em).

Please stay tuned; I will post the patch in here when it's ready.
I posted the patch in bug 891948: https://bugzilla.mozilla.org/attachment.cgi?id=776294, which is tracking Fx 25.
Depends on: 891948
(In reply to Mike de Boer [:mikedeboer] from comment #20)
> I almost have a patch ready that makes sliding of the findbar AND the
> content area very smooth, which *should* satisfy your concerns, I think
I don't think the problem is how the page shifts (smoothly or not). It's that the page shifts at all. Who on UX is pushing for shifting the whole page every time? Who on UR is testing these changes?
Please, just change it back to bottom and move Highlight All and Match Case to near the text field. They are too far away.
Moving the find bar back to the bottom clearly isn't the solution. The "partial" find bar ala Chrome could be a not perfect but still more acceptable solution  especially for those who suffer sickness when the content scrolls down (personally I'm very happy with the current on top implementation).
(In reply to Edward Lee :Mardak from comment #25)
> Created attachment 777307 [details]
> potential covering content that can be scrolled back
We could avoid shifting page content when the find bar appears by having the find bar cover content at the top. To avoid issues of not being able to access content at the top of the page, we would allow content to be scrolled back to the top.

I'm guessing technically it's a bit tricky because even without animations, we need to simultaneously:
1) show the find bar of some height X
2) shrink the content browser element by height X
2) scroll content down by the same X
(In reply to Edward Lee :Mardak from comment #26)
> 3) scroll content down by the same X
Perhaps with animation, we can get the animation frame event to compute how much has animated then contentDocuemnt.scrollTop -= delta; ?
In this case I think covering content is worse than shifting content. No matter the content on the top is important or not (and we shouldn't assume either), covering content is not a desired behavior. Yes we can make the content scrollable like :mardak suggested in comment 26, but that would be a fix because we covered the content in the first place, and the page would shift back when closing the find bar.
 
That being said, I think the current issue is that the shifting is jumpy and not very smooth, especially when closing the find bar, we should work on improving that.

*Also, we shouldn't move the find bar back to the bottom just because we might get away with covering content at the bottom. We decided to move the find bar on top because that's where the rest of our UI sits and where most users would expect it to be (you can argue about that in bug 869543)
(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #28)
> In this case I think covering content is worse than shifting content.
From my mockups, you can scroll back to the "covered content" which is no different than if the user happened to scroll down or resized the window. Just to be clear, you're saying that the find bar should never cover content? The current implementation already covers content at the bottom of the screen when it appears, so if we are to achieve that, we'll need to resize/scale the content to make sure no content is hidden as the find bar appears.
(In reply to Edward Lee :Mardak from comment #29)
> (In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #28)
> > In this case I think covering content is worse than shifting content.
> From my mockups, you can scroll back to the "covered content" which is no
> different than if the user happened to scroll down or resized the window.
> Just to be clear, you're saying that the find bar should never cover
> content? The current implementation already covers content at the bottom of
> the screen when it appears, so if we are to achieve that, we'll need to
> resize/scale the content to make sure no content is hidden as the find bar
> appears.

I meant covering the content when user did not scroll down.
(In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #30)
> (In reply to Edward Lee :Mardak from comment #29)
> > (In reply to Zhenshuo Fang (:fang) - Firefox UX Team from comment #28)
> > > In this case I think covering content is worse than shifting content.
> > From my mockups, you can scroll back to the "covered content" which is no
> > different than if the user happened to scroll down or resized the window.
> > Just to be clear, you're saying that the find bar should never cover
> > content? The current implementation already covers content at the bottom of
> > the screen when it appears, so if we are to achieve that, we'll need to
> > resize/scale the content to make sure no content is hidden as the find bar
> > appears.
> 
> I meant covering the content when user did not scroll down.

Well, we covered content in this same manner when the find bar was at the bottom, except it was the content at the bottom of the scrollview that got covered.

I agree that we should keep content from shifting, and just move the scroll position of the page to compensate for the shorter scroll height.
I quickly hacked together something with DOM Inspector inspecting the xul:browser element for the tab.

target.animate = () => {
  let {height} = target.boxObject;
  let diff = target.lastHeight - height;
  target.contentDocument.documentElement.scrollTop += diff;
  target.lastHeight = height;
  target.ownerGlobal.requestAnimationFrame(target.animate);
}

Then with that checking for animation frames, the attached 10 second video shows:
0s: find bar appears (content doesn't really shift)
1s: find bar hidden (content doesn't shift)
3s: find bar appears again
5s: page scrolled up to top
7s: page scrolled further down
9s: find bar hidden

I'm guessing there might be some rounding issues or partial frame animation because when the find bar appears, you can see the page content jitter just slightly (probably 1px up/down).
Comment on attachment 777525 [details]
video of automatically adjusting scroll

That looks like a great solution to me.
I also think that the solution in Comment 32 is the best way how to solve the issue. Maybe there is some misunderstanding on the term “covering content”. If the viewport for the page is shrunk, a strip of content is always “covered” or rather just not visible because of the new lower height of the viewport. You can scroll back to that content if you want. What I understand as a problem of covering content that should be avoided, is that some part of the content is not visible and you have no means of viewing it apart from closing the Find Bar. This is what Google Chrome essentially does, albeit on a very small area as their find bar is very narrow.

The solution presented above in Comment 32 thus for me and by my definition does not cover content in the way that is needful to be avoided and thus is OK.
(In reply to Edward Lee :Mardak from comment #32)
> Created attachment 777525 [details]
> video of automatically adjusting scroll
> 
> I quickly hacked together something with DOM Inspector inspecting the
> xul:browser element for the tab.

Cool stuff, Edward! I'll check this out as soon as bug 891948 is reviewed and landed.
(In reply to Edward Lee :Mardak from comment #32)
> I'm guessing there might be some rounding issues or partial frame animation
> because when the find bar appears, you can see the page content jitter just
> slightly (probably 1px up/down).

I think a simple way to get around that problem would be to delay the content area adjustment until the animation has ended, and to make the findbar overlap the content area until then.
So for example, for fadein, with a findbar height of 20px:
 1. Animate findbar position from -20px to 0px and findbar margin-bottom from
    0px to -20px, simultaneously, using the same timing functions
 2. When the transition has finished, set findbar margin-bottom to 0px and scroll
    the content page up by 20px.
For fadeout:
 1. Set findbar margin-bottom to 20px and scroll the content page down by 20px.
 2. Animate findbar back up.
(In reply to Markus Stange [:mstange] from comment #36)
> For fadeout:
>  1. Set findbar margin-bottom to 20px and scroll the content page down by 20px.
                                   ^
                                   -20px, of course.

Or do something equivalent using transforms. I don't know how the animation is implemented at the moment.
Attached patch proof of concept (mac only) (obsolete) — — Splinter Review 
Mike, what do you think of this? This patch replaces your patch from bug 891948 but reuses ideas from it. It only contains Mac changes.
Attachment #778965 - Flags: feedback?(mdeboer)
(In reply to Markus Stange [:mstange] from comment #38)
> Created attachment 778965 [details] [diff] [review]
> proof of concept (mac only)
I applied the patch locally... Excellent!

No page scrolling when showing or hiding unless you scrolled to the top of the content before hiding.

For pages that don't have scrollbars, e.g., about:home, the top of the page is covered momentarily then appears when content window/marginTop is adjusted (without being able to compensate with scrollTop).
Comment on attachment 778965 [details] [diff] [review]
proof of concept (mac only)

> findbar[position="top"] {
>+  position: fixed;
>+  width: 100%;

See bug 891948 comment 12.

I played around with this in DOM Inspector and position:absolute; width:100% seems to work if the parent node has position:relative. I'm not sure if setting position:relative on .browserContainer all the time (rather than only during the transition) would have some undesirable overhead. Either way it's somewhat messy because it would add an external dependency to the find bar binding.
Comment on attachment 778965 [details] [diff] [review]
proof of concept (mac only)

Markus, I really like the effect you accomplished and it works like a charm when applied locally (OSX for me)!

Dão already explained in comment 40 that position: fixed is kinda problematic, so we need to find another solution - perhaps we'll end up with Dão's experiment with position: absolute(?)

More focused on your patch:
* I like your use of translateY, instead of my fiddling with transition properties.
* we usually don't use `let self = this` anymore, but rather `function(){}.bind(this)` to adjust the scope.
* generally, it's a good practice to supply the radix to parseInt: `parseInt(x, 10);`
Attachment #778965 - Flags: feedback?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #41)
> * we usually don't use `let self = this` anymore, but rather
> `function(){}.bind(this)` to adjust the scope.
Aww. No new fancy of using "event => { .. }" to autobind functions? ;) Note you need to removeEventListener of the bound function and not the original function.
(In reply to Edward Lee :Mardak from comment #42)
> (In reply to Mike de Boer [:mikedeboer] from comment #41)
> > * we usually don't use `let self = this` anymore, but rather
> > `function(){}.bind(this)` to adjust the scope.
> Aww. No new fancy of using "event => { .. }" to autobind functions? ;) Note
> you need to removeEventListener of the bound function and not the original
> function.

We can use fat-arrow functions, in fact I hear other people (myself included!) prefer them due to the bound-function memory leak that is so easy to introduce when using .bind().
Ah, I should have just said that there is nothing stopping us from using fat-arrow functions in general.

However, when using them in conjunction with event listeners it is still important to make sure that the same function definition is being removed that was added.
Attached patch v1 (obsolete) — — Splinter Review 
I ended up not using an anonymous function (fat arrow or not) after all because I need to be able to cancel the event listener from the close() method, too. So I moved the handler code to findbarBinding.handleEvent.

Other changes:
 - Created toolkit/content/widgets/findbar.css and moved animation and
   positioning stuff there
 - Made the findbar binding set position:relative on the findbar's parent
   element
 - Used scrollBy() instead of scrollTop because for plaintext documents the
   scrollable element is document.body instead of document.documentElement and
   scrollBy() works for both

(In reply to Dão Gottwald [:dao] from comment #40)
> Comment on attachment 778965 [details] [diff] [review]
> proof of concept (mac only)
> 
> > findbar[position="top"] {
> >+  position: fixed;
> >+  width: 100%;
> 
> See bug 891948 comment 12.
> 
> I played around with this in DOM Inspector and position:absolute; width:100%
> seems to work if the parent node has position:relative. I'm not sure if
> setting position:relative on .browserContainer all the time (rather than
> only during the transition) would have some undesirable overhead.

I don't know of any overhead. We'll probably notice if it's the case.

> Either way
> it's somewhat messy because it would add an external dependency to the find
> bar binding.

I made the binding set it, which may be somewhat unexpected for findbar consumers but at least they won't have to do anything special.
Assignee: nobody → mstange
Attachment #778965 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #780286 - Flags: review?(dao)
Component: General → Find Toolbar
Product: Firefox → Toolkit
Wait, can page content JS replace the scrollBy function and have us call into arbitrary content JS? Would that be a security risk?
In a quick test I wasn't able to overwrite the scrollBy function from a content script.
Comment on attachment 780286 [details] [diff] [review]
v1

>+findbar[hidden] {
>+  /* Override display:none to make the transition work. */
>+  display: -moz-box;
>+  visibility: collapse;
>+  opacity: 0;
>+  transition-delay: 0s, 0s, 150ms;
>+  transform: translateY(2em);

Can you just use bottom:-2em here?

>+findbar[position="top"][hidden] {
>+  transform: translateY(-2em);

And top:-2em here?

>+          if (this.getAttribute("position") == "top") {
>+            let offset = parseInt(this._browser.style.marginTop, 10);
>+            this._browser.contentWindow.scrollBy(0, -offset);
>+            this._browser.style.marginTop = "0px";
>+          } else {
>+            this._browser.style.marginBottom = "0px";

this._browser.style.removeProperty?

I haven't actually applied and tested the patch myself yet, will do that later today or tomorrow.
(In reply to Markus Stange [:mstange] from comment #46)
> Wait, can page content JS replace the scrollBy function and have us call
> into arbitrary content JS?

Only if you called contentWindow.wrappedJSObject.scrollBy.
(In reply to Dão Gottwald [:dao] from comment #47)
> Comment on attachment 780286 [details] [diff] [review]
> v1
> 
> >+findbar[hidden] {
> >+  /* Override display:none to make the transition work. */
> >+  display: -moz-box;
> >+  visibility: collapse;
> >+  opacity: 0;
> >+  transition-delay: 0s, 0s, 150ms;
> >+  transform: translateY(2em);
> 
> Can you just use bottom:-2em here?
> 
> >+findbar[position="top"][hidden] {
> >+  transform: translateY(-2em);
> 
> And top:-2em here?

Off-main-thread animation support for transforms is mostly implemented and almost enabled, and I don't know of any plans to implement OMTA for position:absolute offsets. I think it's worth using transforms here, at least until we have OMTA support for top/bottom.

> 
> >+          if (this.getAttribute("position") == "top") {
> >+            let offset = parseInt(this._browser.style.marginTop, 10);
> >+            this._browser.contentWindow.scrollBy(0, -offset);
> >+            this._browser.style.marginTop = "0px";
> >+          } else {
> >+            this._browser.style.marginBottom = "0px";
> 
> this._browser.style.removeProperty?

Good idea.

I also forgot about one issue: if the content page zoom factor is different from the chrome zoom factor, the scrollBy call won't do what we want. I'll deal with that now.

(In reply to Dão Gottwald [:dao] from comment #48)
> (In reply to Markus Stange [:mstange] from comment #46)
> > Wait, can page content JS replace the scrollBy function and have us call
> > into arbitrary content JS?
> 
> Only if you called contentWindow.wrappedJSObject.scrollBy.

Awesome.
Attached patch v2 (obsolete) — — Splinter Review 
Now handles zooming.
Attachment #780286 - Attachment is obsolete: true
Attachment #780286 - Flags: review?(dao)
Attachment #780325 - Flags: review?(dao)
Comment on attachment 780325 [details] [diff] [review]
v2

>+findbar {
>+  transform: translateY(0);

I think you can omit transform: translateY(0);

>+            this.addEventListener("transitionend", this, false);

>+          this.removeEventListener("transitionend", this, false);

you can omit the third argument

>+            case "transitionend":

please check that event.target == this and event.propertyName == "visibility"

>+              // Change the browser size in such a way that the region that's
>+              // overlapped by the findbar can be scrolled to, but try to
>+              // avoid a visual shift of the browser contents.
>+              let [chromeOffset, contentScrollOffset] = this._findOffsets();
>+              if (this.getAttribute("position") == "top") {
>+                this._browser.style.marginTop = chromeOffset + "px";
>+                this._browser.contentWindow.scrollBy(0, contentScrollOffset);
>+              } else {
>+                this._browser.style.marginBottom = chromeOffset + "px";
>+              }

Can we just make the find bar position:static rather than setting a margin on the browser?
Comment on attachment 780325 [details] [diff] [review]
v2

Looks good, but I'd like to see a new patch with my previous comment addressed (the position:static part in particular).
Attachment #780325 - Flags: review?(dao) → review-
How did you want to shift the content with findbar position:static? Using negative margin-bottom to pull up the content? I believe there's layering/z-index type issues when I played around with that before.

Assuming we can deal with the layering, the code would need to constantly adjust the margin-bottom of the findbar to have content shift up at the same rate of the findbar animating down then do the transitionend adjustment with scrollBy. ?
You seem to be misunderstand the code I quoted in comment 51. It runs when the transition is done, not while transitioning.
(In reply to Dão Gottwald [:dao] from comment #51)
> Comment on attachment 780325 [details] [diff] [review]
> v2
> 
> >+findbar {
> >+  transform: translateY(0);
> 
> I think you can omit transform: translateY(0);

I agree.

> 
> >+            this.addEventListener("transitionend", this, false);
> 
> >+          this.removeEventListener("transitionend", this, false);
> 
> you can omit the third argument

oh good

> 
> >+            case "transitionend":
> 
> please check that event.target == this and event.propertyName == "visibility"

will do

> 
> >+              // Change the browser size in such a way that the region that's
> >+              // overlapped by the findbar can be scrolled to, but try to
> >+              // avoid a visual shift of the browser contents.
> >+              let [chromeOffset, contentScrollOffset] = this._findOffsets();
> >+              if (this.getAttribute("position") == "top") {
> >+                this._browser.style.marginTop = chromeOffset + "px";
> >+                this._browser.contentWindow.scrollBy(0, contentScrollOffset);
> >+              } else {
> >+                this._browser.style.marginBottom = chromeOffset + "px";
> >+              }
> 
> Can we just make the find bar position:static rather than setting a margin
> on the browser?

The problem with that is that it might wiggle the page contents when the findbar has a fractional height. The margins I set are carefully adjusted to be of integer device pixels, and the findbar height isn't. If the fractional device pixel offset of the browser changes, pixel snapping in the contained page might end up differently on different page elements, and the result looks a little wiggly.
If the margin you set doesn't match the find bar height, then there'd be a gap between the find bar and the content area or a thin row at the top of the content area would be hidden and inaccessible, neither of which seems desirable either.
Yes. There won't be a gap because I use floor() in the place where it matters. There might be a 1px strip that is covered. How bad would that be? In my opinion that wouldn't matter much, and it's something that would only happens while the findbar is visible.
I guess the really clean solution would be to add a SetTemporaryScrollMargins API that does all the coordinate conversions internally, and does both the scrollbox shifting and the scroll position offset in a place where it has access to fractional values for everything. We would probably also need something like that for E10s where we can't simply call scrollBy on a page in a content process from chrome.
But I don't really want to go down that rabbit hole just yet...
(In reply to Markus Stange [:mstange] from comment #57)
> There might be a 1px strip that is covered. How bad would that be?

Depends on the page. It's the kind of quirk I think we should avoid on principle.
So, how about this: Before starting a findbar transition, we set .style.height on the findbar in such a way that its height is an integer number of device pixels. Then we follow your suggestion with position:static and everything's fine.
Sounds ok to me.
Attached patch v3 (obsolete) — — Splinter Review 
Addressed comments, except for aEvent.propertyName == "visibility" where I had to use aEvent.propertyName == "transform" instead, because visibility isn't transitioned. Visibility is applied right at the beginning of the animation without any transition-delay or -duration, so there's no transitionend event for it.
Attachment #780325 - Attachment is obsolete: true
Attachment #782629 - Flags: review?(dao)
Hey everybody - showing up late to the party, just wondering if the case that this patch partially deals with (the find bar accidentally covering content that we're interested in), is anything more than a really unlikely edge-case.

And I know it's a nasty case - but there are work-arounds. Like, I just got a demonstration from beltzner where the Chrome findbar will move out of the way when it detects that the result that is highlighted is underneath it. (Although, Chrome can get away with this because it's got a smaller find UI...but we could do something similar, could we not?).

Talking with :ehsan in meatspace, it sounds like he's concerned about the page jumping in the case where the user has opened the find-bar, scrolled up to the top of the page that was originally overlayed, and then *closing* the find-bar. We would, of course, snap the browser back up to its original size, and that would cause a jump. Is this something we should be concerned about?

(Ehsan has a gun to my head)
What ever you decide, please remember the findbar lives in /toolkit/ and there are other consumers (like, us, SeaMonkey). Any resolution should work with the findbar whether it's at the top or at the bottom of the browser. Thanks muchly.
>(In reply to Quicksaver from comment #7)
> by default you can only close the findbar by clicking on the X or by going to the menu
> entry.

Or hitting the Esc key or waiting for it to just go away if you're using it with searches triggered by '/'.... having page content shift in the latter case is _extremely_ jarring...
(In reply to Boris Zbarsky (:bz) from comment #65)
> Or hitting the Esc key or waiting for it to just go away if you're using it
> with searches triggered by '/'.... having page content shift in the latter
> case is _extremely_ jarring...

I admit I hadn't considered those cases, and I have to agree you have a point there.
(In reply to Philip Chee from comment #64)
> What ever you decide, please remember the findbar lives in /toolkit/ and
> there are other consumers (like, us, SeaMonkey). Any resolution should work
> with the findbar whether it's at the top or at the bottom of the browser.
> Thanks muchly.

Agreed. In bug 869543 I only introduced the minimal amount of toolkit changes to move the findbar to the top, which is - as soon as the iterations on this bug are done - the naive approach. All this behavior is tucked behind the 'position' attribute, which now only has the 'top' value implemented. The default is and will be 'bottom'.

Markus, I think it'd be a good idea to do one of the following (on order of my own preference, which is often most influenced by laziness ;) ):
1) only apply the animation and scroll offset corrections in the case of 'position="top"'
2) let the findbar start emitting events when it starts showing/ hiding and move the animation and scroll offset corrections to browser.js.

I think 1) would require the least amount of change in your current approach and safeguard that find bars without position=top will still work as expected. To dev-test this locally, I'd recommend using the 'old' view-source: toolkit/components/viewsource/content/viewSource.xul (see https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=869543&attachment=773881)
(In reply to Philip Chee from comment #64)
> What ever you decide, please remember the findbar lives in /toolkit/ and
> there are other consumers (like, us, SeaMonkey).
Oh, you got rid of the find dialog finally? /ducksandcovers/
(In reply to Mike Conley (:mconley) from comment #63)
> Talking with :ehsan in meatspace, it sounds like he's concerned about the
> page jumping in the case where the user has opened the find-bar, scrolled up
> to the top of the page that was originally overlayed, and then *closing* the
> find-bar. We would, of course, snap the browser back up to its original
> size, and that would cause a jump. Is this something we should be concerned
> about?

This case, and the case where the page is too short to require scrolling, are the two remaining flaws of this approach. I'd love to have a differently-working findbar where these cases are not an issue, but I'm not the right guy to work out the UX for it.
I don't know if the failure cases happen often enough to worry about them. We probably need to run with the current approach for a few weeks until we know.

(In reply to Mike de Boer [:mikedeboer] from comment #67)
> Markus, I think it'd be a good idea to do one of the following (on order of
> my own preference, which is often most influenced by laziness ;) ):
> 1) only apply the animation and scroll offset corrections in the case of
> 'position="top"'
> 2) let the findbar start emitting events when it starts showing/ hiding and
> move the animation and scroll offset corrections to browser.js.

I don't see the need to do this. I've tested the current patch in both top and bottom mode and it works as expected in both. The patch brings one minor negative change for bottom findbars: When the findbar is visible and the page has been scrolled to the very bottom, closing it will now move the page down in one step, and not animated as before.
(In reply to Markus Stange [:mstange] from comment #69)
> I don't see the need to do this. I've tested the current patch in both top
> and bottom mode and it works as expected in both. The patch brings one minor
> negative change for bottom findbars: When the findbar is visible and the
> page has been scrolled to the very bottom, closing it will now move the page
> down in one step, and not animated as before.

Cool! In that case I think we've got Philips' case covered.
Comment on attachment 782629 [details] [diff] [review]
v3

>+findbar {
>+  transition-property: transform, opacity, visibility;
>+  transition-duration: 150ms, 150ms, 0s;
>+  transition-timing-function: ease-in-out, ease-in-out, linear;
>+
>+  /* We position the findbar absolutely so that it can overlap its browser
>+   * during the transition. The findbar binding is responsible for setting
>+   * position:relative on our parent element so that this can work.
>+   */
>+  position: absolute;
>+  left: 0;
>+  right: 0;
>+  bottom: 0;
>+}

We can keep the code messing with 'position' on the find bar and its parent more self-contained if you can drop position:absolute here...

>+        // The findbar has position:absolute so that it can overlay its
>+        // siblings. In order for that to work in XUL, there needs to be an
>+        // ancestor node that has position:relative.
>+        this.parentNode.style.position = "relative";

And remove this...

>           if (this.hidden) {
>             this.hidden = false;
> 
>+            // Set a height on the findbar that's at least as much as the
>+            // current height, but guaranteed to be an integer number of
>+            // screen pixels.
>+            // This way, setting position:static on the findbar after the
>+            // fade in animation won't cause the browser contents to wiggle.
>+            let [chromeOffset, contentScrollOffset] = this._findOffsets();
>+            this.style.height = chromeOffset + "px";
>+            this._contentScrollOffset = contentScrollOffset;
>+
>+            // Wait for the findbar appearance animation to end before
>+            // changing the browser size.
>+            this.addEventListener("transitionend", this);

And set both here...

>           this.hidden = true;
>+
>+          // If the findbar is being hidden before the fade-in animation has
>+          // completed, cancel any pending browser size change.
>+          this.removeEventListener("transitionend", this);

Add the event listener here instead of removing it...

>+          // Revert browser scroll shift + findbar static positioning.
>+          if (this.getAttribute("position") == "top" &&
>+              this.style.position == "static") {
>+            this._browser.contentWindow.scrollBy(0, -this._contentScrollOffset);
>+          }

Check this.style.position != "absolute" here...

>+          this.style.removeProperty("position");

Again set both position styles here...

>+            case "transitionend":
>+              if (aEvent.target == this && aEvent.propertyName == "transform") {
>+                this.removeEventListener("transitionend", this);
>+
>+                // Change the browser size in such a way that the region that's
>+                // overlapped by the findbar can be scrolled to, but try to
>+                // avoid a visual shift of the browser contents.
>+                this.style.position = "static";

Call removeProperty for both here...

>+                if (this.getAttribute("position") == "top") {
>+                  this._browser.contentWindow.scrollBy(0, this._contentScrollOffset);
>+                }

And check !this.hidden here.
Attachment #782629 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #71)
> We can keep the code messing with 'position' on the find bar and its parent
> more self-contained

I've tested your corrections, and the result is unsatisfying: When I toggle the parentNode's position value (or, for that matter, its overflow value), the whole content viewport is repainted. For pages with complex content this can cause noticeable jank. Having position:relative on all the time is more invasive but avoids this jank.
Also, it seems we need to set overflow:hidden on the parentNode, too. Previously I only tested the patch on the UX branch, and overflow:hidden doesn't seem to be necessary there (maybe due to a different navbar z-index?), but on mozilla-central the current patch causes the findbar to move over the browser toolbar during the transition.
Toggling overflow between "visible" and "hidden" also causes everything to be repainted, so it looks like we'll need to set overflow:hidden in the constructor, too.
(In reply to Markus Stange [:mstange] from comment #72)
> (In reply to Dão Gottwald [:dao] from comment #71)
> > We can keep the code messing with 'position' on the find bar and its parent
> > more self-contained
> 
> I've tested your corrections, and the result is unsatisfying: When I toggle
> the parentNode's position value (or, for that matter, its overflow value),
> the whole content viewport is repainted. For pages with complex content this
> can cause noticeable jank. Having position:relative on all the time is more
> invasive but avoids this jank.

I think I'd still prefer those changes, with the removeProperty call disabled with an explaining comment.

> Also, it seems we need to set overflow:hidden on the parentNode, too.
> Previously I only tested the patch on the UX branch, and overflow:hidden
> doesn't seem to be necessary there (maybe due to a different navbar
> z-index?), but on mozilla-central the current patch causes the findbar to
> move over the browser toolbar during the transition.

overflow:hidden on the content area container seems like a sledgehammer and scares me a bit more than position:relative, so I think we need to live with that behavior for now. Maybe we can find a different workaround, but that shouldn't block landing this patch.
(In reply to Dão Gottwald [:dao] from comment #73)
> (In reply to Markus Stange [:mstange] from comment #72)
> > (In reply to Dão Gottwald [:dao] from comment #71)
> > > We can keep the code messing with 'position' on the find bar and its parent
> > > more self-contained
> > 
> > I've tested your corrections, and the result is unsatisfying: When I toggle
> > the parentNode's position value (or, for that matter, its overflow value),
> > the whole content viewport is repainted. For pages with complex content this
> > can cause noticeable jank. Having position:relative on all the time is more
> > invasive but avoids this jank.
> 
> I think I'd still prefer those changes, with the removeProperty call
> disabled with an explaining comment.

Sounds good. This will still cause one unnecessary repaint when the findbar is first opened (per tab?), but I guess that's ok.
Attached patch v4 — — Splinter Review 
I think you should have another look at this before I land it. I had to add a flush and some changes in the transitionend listener.

https://tbpl.mozilla.org/?tree=Try&rev=165018fdd996
Attachment #782629 - Attachment is obsolete: true
Attachment #786990 - Flags: review?(dao)
Comment on attachment 786990 [details] [diff] [review]
v4

>+            // Use position:absolute during the transition.
>+            this.style.position = "absolute";
>+            this.parentNode.style.position = "relative";
>+
>+            // Apparently a flush is necessary after setting position:relative
>+            // on our parentNode, otherwise setting hidden to false won't
>+            // animate the transform change.
>+            this.getBoundingClientRect();

That seems pretty weird to me. Would be nice if we could avoid this, but for now it's ok.

>+            case "transitionend":
>+              // This event fires when the findbar has finished sliding in or
>+              // out. For the fade-in animation, visibility is immediately set
>+              // to visible and only the transform property is animated. For
>+              // the fade-out animation, transform is animated first and then
>+              // visibility is set to "collapsed" when that's finished. So for
>+              // fadeout, we can't actually remove position:absolute from the
>+              // findbar until the visibility change has happened. That's why
>+              // we check for different propertyNames depending on the animation
>+              // direction.
>+              if (aEvent.target == this &&
>+                  aEvent.propertyName == (this.hidden ? "visibility" : "transform")) {
>+                this.removeEventListener("transitionend", this);

I'm not sure I understand this. What happens if you use the "transform" check in both cases?
Attachment #786990 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #76)
> I'm not sure I understand this. What happens if you use the "transform"
> check in both cases?

Then the findbar slides away, briefly appears at its opened place (shifting the browser down) and then vanishes (shifting the browser back up). I suppose transform simply finishes its transition before visibility, so if we set position:static from on transform-transitionend, there is be a brief state of position:static + visibility:visible.
Comment on attachment 786990 [details] [diff] [review]
v4

>+findbar {
>+  transition-property: transform, opacity, visibility;
>+  transition-duration: 120ms, 120ms, 0s;

>+findbar[hidden] {
>+  /* Override display:none to make the transition work. */
>+  display: -moz-box;
>+  visibility: collapse;
>+  opacity: 0;
>+  transition-delay: 0s, 0s, 150ms;

The transition-delay for visibility should be 120ms, right? Does this address comment 77?
Depends on: 903560
Depends on: 903564
Depends on: 903586
Depends on: 903615
https://hg.mozilla.org/mozilla-central/rev/452ca8b779f3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 891948
No longer depends on: 891948
This site still does a page jump with the patch.

https://github.com/explore
Depends on: 903939
(In reply to comment #81)
> This site still does a page jump with the patch.
> 
> https://github.com/explore

Yeah, it doesn't really work for any page with a height less than that of the viewport, *including* pages which use an iframe to display most of the content (such as Gmail.)  And it also doesn't prevent the page from jumping up when you scroll content down on long pages and close the find bar.

I don't personally like this fix, but the current patch does what it was intended to do.  I just think that the intention of the patch is what's broken.  ;-)
(In reply to comment #82)
> (In reply to comment #81)
> > This site still does a page jump with the patch.
> > 
> > https://github.com/explore
> 
> Yeah, it doesn't really work for any page with a height less than that of the
> viewport, *including* pages which use an iframe to display most of the content
> (such as Gmail.)  And it also doesn't prevent the page from jumping up when you
> scroll content down on long pages and close the find bar.

And actually, on such pages I think the new behavior is worse than what we had before, because first there is an animation and then the entire page shifts down when the animation finishes.

I really encourage the proponents of this fix to compare our behavior against Chrome's find bar. :(
Just placing FindBar back to bottom fixes every thing.
Gavin, I really don't think this is the right fix as explained in the above conversation.  What's your take on this as the Module Owner?

Thanks!
Flags: needinfo?(gavin.sharp)
I'm not sure what you're asking - are you suggesting that we need to give up on moving the find bar? Or spend more time making it work like Chrome? Or something else? Are your issues covered by the existing bugs on file?

(Can we consolidate the issues-with-findbar-on-top tracking by making them all block bug 869543? I'm not sure I understand why this bug depends on a bunch of other bugs.)
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86)
> I'm not sure what you're asking - are you suggesting that we need to give up
> on moving the find bar? Or spend more time making it work like Chrome? Or
> something else? Are your issues covered by the existing bugs on file?

I think we should make the find bar cover the content area and never have it shift the content up or down, which is what Chrome does.  Chrome's find bar doesn't span the whole width of the browser and does smart tricks to make sure that it doesn't overlay the found text in the edge case where it is located at the very top of the page.  I'd be fine with handling that edge case somehow (even if it means shifting the page down *when* that edge case happens) but I think our current implementation even with this fix where we end up shifting down the content in some cases is very suboptimal.

As a bit of a backstory, I filed bug 893131 which got duped to this one.  See comment 16.  Then we sought UX feedback, and the feedback recommended shifting content as opposed to covering it, but I am not convinced that these are the only available options that we have to pick from -- we can easily handle that edge case, by, for example, shifting content (see comment 28).  Then the current design was proposed (see comment 32 onwards) but because of the reasons I described in comment 82 I don't believe that is effective in the general case.

I could go on and file another bug, but before doing that I'd like to know what your take is on this whole issue: is this current fix good enough, do we want to avoid shifting content, or do you think shifting content is acceptable and we shouldn't do anything about it?

Thanks!
Flags: needinfo?(gavin.sharp)
Seems like a discussion best had on firefox-dev, rather than a bug. Do you want to start a thread there?

I'm missing a lot of context, so it would be useful if a summary could be posted of e.g. why we decided a solution like Chrome's was undesirable, what the known outstanding bugs are and what our plans are to address them, etc.

The animation in today's Nightly does seem a bit janky, but not overly so. It seems like your reaction is based on the change of behavior rather than the inherent badness of the new solution, but that's something we need to take into account too.
Flags: needinfo?(gavin.sharp)
(In reply to comment #88)
> Seems like a discussion best had on firefox-dev, rather than a bug. Do you want
> to start a thread there?

Sure.

> I'm missing a lot of context, so it would be useful if a summary could be
> posted of e.g. why we decided a solution like Chrome's was undesirable, what
> the known outstanding bugs are and what our plans are to address them, etc.

I don't have that context, and in fact I am really curious to know the reasons behind the current decisions myself!

> The animation in today's Nightly does seem a bit janky, but not overly so.

I have been disregarding the performance of the animation for now.  ;-)

> It
> seems like your reaction is based on the change of behavior rather than the
> inherent badness of the new solution, but that's something we need to take into
> account too.

No not at all, but more on that in the thread.
Gavin, Ehsan, if you have not seen this thread it has some discussion and context I believe.  

https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000658.html
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #90)
> Gavin, Ehsan, if you have not seen this thread it has some discussion and
> context I believe.  
> 
> https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000658.html

Oops, guess I should have posted from the beginning of the thread:
https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000646.html
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #82)
> Yeah, it doesn't really work for any page with a height less than that of
> the viewport, *including* pages which use an iframe to display most of the
> content (such as Gmail.)  And it also doesn't prevent the page from jumping
> up when you scroll content down on long pages and close the find bar.

In the Gmail case, a possible solution is to fix the scroll position of each frame in addition to the main page.

In the case of short pages, we could fix the min-height of the page to the height of the viewport. That way, the extra whitespace would be part of the page, and all of that could be scrolled up as the find bar comes down. In other words, if the page is too short make it taller. This is, admittedly, kind of a hack, though it might be preferable to having any cases where the page shifts. As a side note, let me point out that, with short pages, you’re somewhat less likely to run a find, since you can see the entire page all at once.

By the way, I checked Safari’s behaviour on Windows, and it fails on both of these cases. Fixing them both in Firefox would go beyond [parity-safari].
Seems like we should definitely uplift this to Aurora. Markus, can you request approval?
tracking-firefox25: ? → +
Comment on attachment 786990 [details] [diff] [review]
v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 869543
User impact if declined: jarring and unexpected page shifts
Testing completed (on m-c, etc.): 1 week on mozilla-central
Risk to taking this patch (and alternatives if risky): low to medium, alternative: backing out bug 869543
String or IDL/UUID changes made by this patch: none
Attachment #786990 - Flags: approval-mozilla-aurora?
Comment on attachment 786990 [details] [diff] [review]
v4

Can't ship an unfinished feature, so approving the forward fix for Aurora. We can always backout in the future.
Attachment #786990 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
QA Contact: manuela.muntean
Attached image screenshot.png — 
For the pre-beta sign-off of the Find bar redesign feature I've tested the following scenarios, with latest Aurora (build ID: 20130905004001) on Ubuntu 12.10 32bit and Win 8 64bit, and everything seems to work as expected:

- open the find bar in a too short page to require scrolling (about:home)
- CTRL + F, scroll until the end, hit ESC
- the find bar does cover the top part for some pages (ex: https://www.mozilla.org/en-US/about/), but then again, it doesn't for others (ex: results of a Google search); but even if it covers the top part, when searching a word from that part, the word is found, showed and highlighted
- scroll down a page, CTRL + F, scroll up, hit ESC
- the URL from comment 81: https://github.com/explore, still seems to have a page jump (see the scrollbar in the attached screenshot), but only if I open the find bar when I'm at the top at the page (if I scroll down to the end of the page and the press CTRL + F, I don't see a page jump (no movement of the scrollbar))
Blocks: 914180
(In reply to David Regev from comment #92)
> In the case of short pages, we could fix the min-height of the page to the
> height of the viewport. That way, the extra whitespace would be part of the
> page, and all of that could be scrolled up as the find bar comes down. In
> other words, if the page is too short make it taller. This is, admittedly,
> kind of a hack, though it might be preferable to having any cases where the
> page shifts. As a side note, let me point out that, with short pages, you’re
> somewhat less likely to run a find, since you can see the entire page all at
> once.

By the way, I was able to work around the short-page issue with a little CSS, as detailed in bug 903564 comment 2. So it does seem to be technically possible to fix that issue and not back out this feature.
Depends on: 915178
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131009004001).
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 SeaMonkey/2.24

With the page for this bug report positioned at comment #37, the comment header positioned at the very top of the viewport, my cursor clicked on text in that comment, I select Ctrl-F.  The page jumps down as the Find toolbar appears.
(In reply to David E. Ross from comment #100)
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 SeaMonkey/2.24

I'm unable to reproduce this here on Ubuntu with Firefox 27. That said, this bug was resolved and verified fixed two releases ago. Please file a new bug report with your steps to reproduce.

Thank you.
Instead of a new bug, I added a comment to bug #248715.  I also suggested that bug is a Toolkit problem, not Firefox.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: