Closed Bug 858687 Opened 12 years ago Closed 12 years ago

BrowserToolbar's menu button is optional

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox22- wontfix)

RESOLVED FIXED
Firefox 23
Tracking Status
firefox22 - wontfix

People

(Reporter: sriram, Assigned: lucasr)

References

Details

Attachments

(3 files, 1 obsolete file)

Since BrowserToolbar's menu button is optional and doesn't have any curve, we can base the entire layout with tabs-button as the right most element, if the menu button isn't present. This makes us have one single browser_toolbar file that is independent of whether we show menu button or not.
This merges the browser_toolbar files into one. However the animation is a bit messed up. Lucas, could you take a look at this?
Flags: needinfo?(lucasr.at.mozilla)
Attached patch Patch: One BrowserToolbar (obsolete) — Splinter Review
This uses dimens.xml to reduce the browser_toolbar to just one file shared between portrait and landscape mode on phones. This can be used only if the animation in the previous WIP is fixed.
Blocks: 848719
Taking this bug. I'm making massive changes to the original patch.
Assignee: nobody → lucasr.at.mozilla
Flags: needinfo?(lucasr.at.mozilla)
You don't have to do it for the landscape mode. I have the landscape mode removed in Bug 861138. Please base your patch on top it. And please r+ the other one if you can, so that I can land ;)
Comment on attachment 733968 [details] [diff] [review] Patch: One BrowserToolbar As per bug 861138, we are removing the landscape mode. So, once this bug uses one browser_toolbar irrespective of menu button, we are done.
Attachment #733968 - Attachment is obsolete: true
(In reply to Sriram Ramasubramanian [:sriram] from comment #4) > You don't have to do it for the landscape mode. I have the landscape mode > removed in Bug 861138. Please base your patch on top it. And please r+ the > other one if you can, so that I can land ;) Sure. I'm only changing layout/browser_toolbar_menu.xml and layout-large-v11/browser_toolbar_menu.xml. I'll rebase as soon as I stabilize my changes.
This patch does a major simplification on our browser toolbar layout: - browser_toolbar.xml for both phones and tablets have a much simpler layout now with much less nested views. - The animation to expand the entry is greatly simplified because the animation doesn't require layout updates anymore. No hardcoded values are needed. - The sidebar animation is slightly simplified due to the new way we position the right edge view. - No curves are needed on BrowserToolbarBackground anymore. Removed all the code to related to curves in it. - No need for the custom view for RightEdge anymore as it doesn't need to match the toolbar background anymore. Removed. - Forward button animation is slightly simpler with the new layout. - Removed a few of dimens/style resources that are not necessary anymore. This is just a first round of cleanups. I'll probably remove more cruft while working on the necessary changes for the new about:home behavior.
Attachment #741418 - Flags: review?(mark.finkle)
Comment on attachment 741418 [details] [diff] [review] Major simplification of browser toolbar layout This is pretty big, but looks sane. Make sure you test on some phones and tablets.
Attachment #741418 - Flags: review?(mark.finkle) → review+
Attachment #741403 - Flags: review?(mark.finkle) → review+
Turns out the backout caused more bustage. Backed out harder. https://hg.mozilla.org/mozilla-central/rev/8058ffec4f32
Blocks: 866110
Do we need this in FF22 in support of bug 848719?
Flags: needinfo?(sriram_r)
Flags: needinfo?(lucasr.at.mozilla)
Its good to have. But this would have dependencies on some other bugs. Lucas might be able to answer the dependencies.
Flags: needinfo?(sriram_r)
(In reply to Alex Keybl [:akeybl] from comment #16) > Do we need this in FF22 in support of bug 848719? Yes, but this is a fairly complex change that is probably not Beta material at this point. Is bug 848719 a release blocker or something?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #18) > (In reply to Alex Keybl [:akeybl] from comment #16) > > Do we need this in FF22 in support of bug 848719? > > Yes, but this is a fairly complex change that is probably not Beta material > at this point. Is bug 848719 a release blocker or something? Didn't want their to be per-version inconsistencies. Sounds like this isn't a critical change, or worth taking in our final beta.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: