BrowserToolbar's menu button is optional

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: sriram, Assigned: lucasr)

Tracking

unspecified
Firefox 23
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22- wontfix)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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)
Posted 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.
Reporter

Updated

6 years ago
Blocks: 848719
Assignee

Comment 3

6 years ago
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
Assignee

Comment 6

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

Comment 8

6 years ago
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)
Assignee

Comment 18

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.