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)
Tracking
(firefox22- wontfix)
RESOLVED
FIXED
Firefox 23
People
(Reporter: sriram, Assigned: lucasr)
References
Details
Attachments
(3 files, 1 obsolete file)
36.23 KB,
patch
|
Details | Diff | Splinter Review | |
3.37 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
91.72 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
Taking this bug. I'm making massive changes to the original patch.
Assignee: nobody → lucasr.at.mozilla
Flags: needinfo?(lucasr.at.mozilla)
Reporter | ||
Comment 4•12 years ago
|
||
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 ;)
Reporter | ||
Comment 5•12 years ago
|
||
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•12 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 7•12 years ago
|
||
Attachment #741403 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•12 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 9•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #741403 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Comment 11•12 years ago
|
||
Backed out due to Robocop failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00d40e2c278
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f111af2d094
Pushed a possible fix to try:
https://tbpl.mozilla.org/?tree=Try&rev=8a4840dd5b13
Assignee | ||
Comment 12•12 years ago
|
||
Tests now pass with a small view id fix on the tests.
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f70c5a5f09
https://hg.mozilla.org/integration/mozilla-inbound/rev/039a1de069ed
Assignee | ||
Comment 13•12 years ago
|
||
Pushed again now that the test infra is fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08dec3bd99a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd9b6bf7e2d
Comment 14•12 years ago
|
||
Turns out the backout caused more bustage. Backed out harder.
https://hg.mozilla.org/mozilla-central/rev/8058ffec4f32
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/08dec3bd99a0
https://hg.mozilla.org/mozilla-central/rev/bcd9b6bf7e2d
https://hg.mozilla.org/mozilla-central/rev/f554f49e1ffa
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Updated•11 years ago
|
status-firefox22:
--- → affected
tracking-firefox22:
--- → ?
Comment 16•11 years ago
|
||
Do we need this in FF22 in support of bug 848719?
Flags: needinfo?(sriram_r)
Flags: needinfo?(lucasr.at.mozilla)
Reporter | ||
Comment 17•11 years ago
|
||
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•11 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)
Comment 19•11 years ago
|
||
(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.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•