914180 - Backout moving findbar to the top

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

After bug 869543 landed and moved to the top of the browser element, several issues arose, among which bug 893446. However the fix for that bug, overlaying the findbar on top of the web page content, rose concerns about obscuring find matches below the findbar (in the top of the page).

There are no technical limitations to fixing that concern, but it will take time to implement, review, test, land and uplift.

The archive of the discussion about the findbar-on-top feature and some background and details behind the decision to back it out can be found at https://mail.mozilla.org/pipermail/firefox-dev/2013-August/thread.html#749.

This bug is about the technical details (say, patches) of backing out the findbar on top, which will move it back to the bottom. Feel free to join in on the discussion on the firefox-dev mailing list!

Comment 1

[Mike de Boer [:mikedeboer]]

After this, bug 892946 and bug 890613 should be revisited.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Backout rev 60865db5f5dc, bug 869543

`hg qbackout` output for rev 60865db5f5dc.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Backout rev e1bf70be67ec, bug 893011

`hg qbackout` output for rev e1bf70be67ec

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Backout rev 452ca8b779f3, bug 893446

`hg qbackout` output for rev 452ca8b779f3

Comment 5

[Mike de Boer [:mikedeboer]]

Added URL to my firefox-dev post regarding this post.

Comment 6

[Luís Miguel [:quicksaver] (not reading bugmail)]

Request: would it be at all possible to backout only of the actual placement of the find bar on top, which from reading the discussion in the mailing list seems to be the concern here, and leave the rest (styles, scrolling method, animations, etc)? For a few reasons:

1) The new animation design, IMO, is smoother than the old animation system, even with the find bar on the bottom, so it would still benefit from it without the issues still occurring when the find bar is on top.
2) If I were to implement a feature in my FindBar Tweak add-on to place the find bar on top (basically the reverse of the "move it back down" feature that it already does), it could make use of the scrolling system put in place by your patch. Seeing as this scroll-content patch has no effect when the find bar is on the bottom, it would make no functional difference (other than an extra "position" attribute check when toggling the find bar).
3) Building on 2), an add-on with such a feature using the system in these patches could function in a way to gather the wanted user feedback discussed in the mailing list posts, without actually "releasing" the feature in the firefox build itself, or making it a hidden setting or anything.

I realize all these changes could be implemented manually in the add-on, but I thought I'd ask anyway, seeing as they wouldn't interfere with firefox's native functioning and could perhaps remain to serve as basis for future work on it if the find bar is to be eventually moved to the top.

Comment 7

[u279076]

Manuela Muntean is the QA Contact for this feature. Adding her so she is in the loop.

Comment 8

[Florian Bender]

Yeah, I'd like to back Comment 6. The "new" findbar is too beatiful to back out entirely, just move it back down and leave the rest, please?

Comment 9

[Luís Miguel [:quicksaver] (not reading bugmail)]

No no, I believe you are misunderstanding this bug. The "new" find bar will remain regardless of this backout, from what I understand. What I was asking about is not the "new" find bar, but rather all the animation work done that came after bug 869543, as well as the CSS for styling the find bar on top (borders, background, etc).

Comment 10

[Justin Dolske [:Dolske]]

Putting on tracking radar -- currently this is only on Nightly+Aurora (26+25). We should get things backed out in the next few days, ideally. (Monday being the uplift and first beta-25 build, which would be best to avoid).

Comment 11

[Justin Dolske [:Dolske]]

(In reply to Florian Bender from comment #8)
> Yeah, I'd like to back Comment 6. The "new" findbar is too beatiful to back
> out entirely, just move it back down and leave the rest, please?

AIUI the 3 bugs referenced (bug 869543, bug 893011, bug 893446), this is only backing out the move to the top (869543), with the other 2 being followups that were made to address the "page jumping". The work that was done in bug 776708 to improve the findbar should be unaffected.

Comment 12

[Justin Dolske [:Dolske]]

Hmm, I suppose my last comment could be ambiguous. Let me try to clarify: the backout is for 3 bugs, all relating to moving the findbar to the top. This rolls us back to having the bug 776708 improvements, but not a findbar at the top.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 801579 [details] [diff] [review]
Backout rev 60865db5f5dc, bug 869543

Dão, I flag you for review for these patches, because I think it's required before requesting uplift to Aurora ;)

As Justin pointed out, it'd be awesome if we could do this before coming Monday to prevent uplift to Beta!

Comment 14

[Dão Gottwald [:dao]]

What's the plan for bug 890613?

Comment 15

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Justin Dolske [:Dolske] from comment #12)
> Hmm, I suppose my last comment could be ambiguous. Let me try to clarify:
> the backout is for 3 bugs, all relating to moving the findbar to the top.
> This rolls us back to having the bug 776708 improvements, but not a findbar
> at the top.

Yes but some of the work in those bugs, in particular the fade-in/out animations, also include changes for the findbar on the bottom, which like I said IMO are performing better than before. But because of the time constraints to apply this backout before the next beta, I can see why making a "selective" backout can be hard.

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #14)
> What's the plan for bug 890613?

See comment 1. I plan to fix 890613, I'm investigating it at this moment.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: All three patches folded into one

Carrying over r=dao. Please use this patch for check-ins.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 804326 [details] [diff] [review]
All three patches folded into one

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 869543
User impact if declined: non-optimal UX of an incomplete findbar-on-top implementation. This patch moves the findbar back to its previous position; the bottom.
Testing completed (on m-c, etc.): None, this is a backout. Try run performed (see next comment)
Risk to taking this patch (and alternatives if risky): none.
String or IDL/UUID changes made by this patch: none.

Comment 19

[Mike de Boer [:mikedeboer]]

Mentioned try run: https://tbpl.mozilla.org/?tree=Try&rev=5acd3a1dbb8d

Comment 20

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Comment on attachment 804326 [details] [diff] [review]
> All three patches folded into one
> 
> [Approval Request Comment]
...
> Risk to taking this patch (and alternatives if risky): none.

It regresses bug 890613. We'll need to uplift bug 821687 to address that.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6dcc6fc612e4

Will uplift to Aurora when this is at least green on fx-team.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Turns out this was supposed to land on Aurora only. Backed out from fx-team:
https://hg.mozilla.org/integration/fx-team/rev/bfed29c24ac5

Landed on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/9bbb8bade260

Comment 23

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

I think we should do this on trunk as well, and suggested as much on firefox-dev. No objections so far. Mike, do you concur?

Comment 24

[Manuela Muntean [Away]]

Verified as fixed with latest Aurora (build ID: 20130916004002) on: Win 8 32bit, Mac OS X 10.8.4 and Ubuntu 12.10 32bit: findbar is at the bottom of the browser, and its new functionallity remains available.

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Backout rev 452ca8b779f3, bug 893446

Rebased on top of m-c/ bug 666816. Carrying over r=dao.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: All three patches folded into one

Rebased on top of m-c/ bug 666816. Carrying over r=dao. Please use this patch for check-ins.

Comment 27

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/c36f479a9755

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Next time, please include this bug # in the commit message (like I did for the Aurora push).
https://hg.mozilla.org/mozilla-central/rev/c36f479a9755

Also, I will assume that this has approval for uplifting to Aurora again. The backout landed on Fx25 and now Fx27. I will take care of that ASAP.

Comment 29

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

Yes, a=me for Aurora.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20fe34362674

Comment 31

[Manuela Muntean [Away]]

Verified as fixed with latest Aurora (build ID: 20131007004003) on: Win 7 64-bit, Mac OS X 10.8.4 and Ubuntu 12.10 32bit: the find bar is at the bottom of the browser, and its new functionallity remains available.

Comment 32

[Ekanan Ketunuti]

Verified fixed in Nightly 27.0a1 buildid 20131007030203. The findbar is at the bottom.