Backout moving findbar to the top

VERIFIED FIXED in Firefox 25

Status

()

Firefox
General
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ verified, firefox26+ verified, firefox27 verified)

Details

(Whiteboard: [good first verify], URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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!
(Assignee)

Comment 1

4 years ago
After this, bug 892946 and bug 890613 should be revisited.
(Assignee)

Comment 2

4 years ago
Created attachment 801579 [details] [diff] [review]
Backout rev 60865db5f5dc, bug 869543

`hg qbackout` output for rev 60865db5f5dc.
(Assignee)

Comment 3

4 years ago
Created attachment 801581 [details] [diff] [review]
Backout rev e1bf70be67ec, bug 893011

`hg qbackout` output for rev e1bf70be67ec
(Assignee)

Comment 4

4 years ago
Created attachment 801582 [details] [diff] [review]
Backout rev 452ca8b779f3, bug 893446

`hg qbackout` output for rev 452ca8b779f3
(Assignee)

Comment 5

4 years ago
Added URL to my firefox-dev post regarding this post.
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.
Manuela Muntean is the QA Contact for this feature. Adding her so she is in the loop.
QA Contact: manuela.muntean

Comment 8

4 years ago
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?
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).
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).
status-firefox25: --- → affected
tracking-firefox25: --- → ?
(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.
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.
status-firefox26: --- → affected
tracking-firefox26: --- → ?
(Assignee)

Comment 13

4 years ago
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!
Attachment #801579 - Flags: review?(dao)
(Assignee)

Updated

4 years ago
Attachment #801581 - Flags: review?(dao)
(Assignee)

Updated

4 years ago
Attachment #801582 - Flags: review?(dao)
What's the plan for bug 890613?
(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.
(Assignee)

Comment 16

4 years ago
(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.

Updated

4 years ago
Depends on: 821687

Updated

4 years ago
Attachment #801582 - Flags: review?(dao) → review+

Updated

4 years ago
Attachment #801581 - Flags: review?(dao) → review+

Updated

4 years ago
Attachment #801579 - Flags: review?(dao) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 804326 [details] [diff] [review]
All three patches folded into one

Carrying over r=dao. Please use this patch for check-ins.
Attachment #804326 - Flags: review+
(Assignee)

Comment 18

4 years ago
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.
Attachment #804326 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

4 years ago
Mentioned try run: https://tbpl.mozilla.org/?tree=Try&rev=5acd3a1dbb8d
(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.

Updated

4 years ago
status-firefox24: --- → unaffected
tracking-firefox25: ? → +
tracking-firefox26: ? → +

Updated

4 years ago
Attachment #804326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6dcc6fc612e4

Will uplift to Aurora when this is at least green on fx-team.
Keywords: checkin-needed
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
status-firefox25: affected → fixed
Keywords: verifyme
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?
Flags: needinfo?(mdeboer)
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.
status-firefox25: fixed → verified
(Assignee)

Comment 25

4 years ago
Created attachment 807788 [details] [diff] [review]
Backout rev 452ca8b779f3, bug 893446

Rebased on top of m-c/ bug 666816. Carrying over r=dao.
Attachment #801582 - Attachment is obsolete: true
Attachment #807788 - Flags: review+
Flags: needinfo?(mdeboer)
(Assignee)

Comment 26

4 years ago
Created attachment 807793 [details] [diff] [review]
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.
Attachment #804326 - Attachment is obsolete: true
Attachment #807793 - Flags: review+
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/c36f479a9755
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox27: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Yes, a=me for Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/20fe34362674
status-firefox26: affected → fixed

Updated

4 years ago
Whiteboard: [good first verify]
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.
status-firefox26: fixed → verified

Comment 32

4 years ago
Verified fixed in Nightly 27.0a1 buildid 20131007030203. The findbar is at the bottom.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
Keywords: verifyme

Updated

3 years ago
Blocks: 905667
You need to log in before you can comment on or make changes to this bug.