Closed
Bug 914180
Opened 11 years ago
Closed 11 years ago
Backout moving findbar to the top
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 27
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
(Depends on 1 open bug, )
Details
(Whiteboard: [good first verify])
Attachments
(4 files, 2 obsolete files)
5.94 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
13.08 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
15.38 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
After this, bug 892946 and bug 890613 should be revisited.
Assignee | ||
Comment 2•11 years ago
|
||
`hg qbackout` output for rev 60865db5f5dc.
Assignee | ||
Comment 3•11 years ago
|
||
`hg qbackout` output for rev e1bf70be67ec
Assignee | ||
Comment 4•11 years ago
|
||
`hg qbackout` output for rev 452ca8b779f3
Assignee | ||
Comment 5•11 years ago
|
||
Added URL to my firefox-dev post regarding this post.
Comment 6•11 years ago
|
||
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•11 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?
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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).
Updated•11 years ago
|
status-firefox25:
--- → affected
tracking-firefox25:
--- → ?
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
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.
Updated•11 years ago
|
status-firefox26:
--- → affected
tracking-firefox26:
--- → ?
Assignee | ||
Comment 13•11 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•11 years ago
|
Attachment #801581 -
Flags: review?(dao)
Assignee | ||
Updated•11 years ago
|
Attachment #801582 -
Flags: review?(dao)
Comment 14•11 years ago
|
||
What's the plan for bug 890613?
Comment 15•11 years ago
|
||
(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•11 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•11 years ago
|
Attachment #801582 -
Flags: review?(dao) → review+
Updated•11 years ago
|
Attachment #801581 -
Flags: review?(dao) → review+
Updated•11 years ago
|
Attachment #801579 -
Flags: review?(dao) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Carrying over r=dao. Please use this patch for check-ins.
Attachment #804326 -
Flags: review+
Assignee | ||
Comment 18•11 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•11 years ago
|
||
Mentioned try run: https://tbpl.mozilla.org/?tree=Try&rev=5acd3a1dbb8d
Comment 20•11 years ago
|
||
(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•11 years ago
|
Updated•11 years ago
|
Attachment #804326 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 21•11 years ago
|
||
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
Comment 22•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 24•11 years ago
|
||
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.
Assignee | ||
Comment 25•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c36f479a9755
Comment 28•11 years ago
|
||
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
Closed: 11 years ago
status-firefox27:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 29•11 years ago
|
||
Yes, a=me for Aurora.
Updated•11 years ago
|
Whiteboard: [good first verify]
Comment 31•11 years ago
|
||
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•11 years ago
|
||
Verified fixed in Nightly 27.0a1 buildid 20131007030203. The findbar is at the bottom.
You need to log in
before you can comment on or make changes to this bug.
Description
•