Closed
Bug 770135
Opened 12 years ago
Closed 11 years ago
New PanelUI and toolbar customization (Milestone 1)
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: Unfocused, Assigned: jaws)
References
(Blocks 3 open bugs)
Details
(Keywords: dev-doc-needed)
Attachments
(12 files, 58 obsolete files)
166.14 KB,
patch
|
Unfocused
:
review+
jaws
:
review+
mconley
:
review+
|
Details | Diff | Splinter Review |
112.85 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
Details | Diff | Splinter Review | |
1.37 KB,
patch
|
Details | Diff | Splinter Review | |
6.90 KB,
patch
|
Details | Diff | Splinter Review | |
2.98 KB,
patch
|
Details | Diff | Splinter Review | |
10.29 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
904 bytes,
patch
|
Details | Diff | Splinter Review | |
1.98 KB,
patch
|
Details | Diff | Splinter Review | |
6.50 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
356.34 KB,
patch
|
Details | Diff | Splinter Review | |
6.64 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
This is a bug to explore some fun ideas to do with the Panel UI. The wiki page provides significant context:
https://wiki.mozilla.org/Features/Desktop/Panel_Menu
Everything is hacky, everything is temporary, everything is exploration. And at this point, everything looks ugly.
Reporter | ||
Comment 1•12 years ago
|
||
This is light on interaction stuff, but it does have:
* Widget that has a popout (click the Weather widget)
* Integration with the Add-ons Manager
For technical notes on how addons register widgets, see the big comment in ManifestParser.cpp.
PanelUI.getWidget("weather-indicator") allows playing around with an API similar to whats described in the wiki notes, where you can alter a widget for all windows, or for just one window (via .forWindow()). Only the setter for "disabled" is implemented.
Builds should automagically appear here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-fe19ad22bc7d
Attachment #638318 -
Flags: feedback?(shorlander)
Attachment #638318 -
Flags: feedback?(jaws)
Attachment #638318 -
Flags: feedback?(dtownsend+bugmail)
Attachment #638318 -
Flags: feedback?(dolske)
Assignee | ||
Updated•12 years ago
|
Attachment #638318 -
Flags: feedback?(fryn)
Comment 2•12 years ago
|
||
Comment on attachment 638318 [details] [diff] [review]
Prototype v1
Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------
Not much UX to comment on yet, but it looks like a good start! :)
Observation about the placement in the add-ons manager: The menu/panel is restricted to the viewport but in the actual toolbar it isn't.
Attachment #638318 -
Flags: feedback?(shorlander) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 638318 [details] [diff] [review]
Prototype v1
Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------
I've looked at the patch a couple times but I don't have any feedback to give at this point, except to say that I'm happy to see that the Panel UI is moving forward!1! \o/
Attachment #638318 -
Flags: feedback?(jaws)
Comment 4•12 years ago
|
||
Comment on attachment 638318 [details] [diff] [review]
Prototype v1
Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------
I looked at the patch too, and I echo Jared's sentiment. :)
Attachment #638318 -
Flags: feedback?(fryn) → feedback+
Comment 5•12 years ago
|
||
Comment on attachment 638318 [details] [diff] [review]
Prototype v1
Review of attachment 638318 [details] [diff] [review]:
-----------------------------------------------------------------
Yup, neato. I think we want to have a programmatic way to register widgets at runtime (just pass the JS object directly to the manager I guess). You probably want to get bsmedberg's take on the manifest parsing stuff, the manager could handle reloads itself if in a bit of a clunky way.
Presumably you're also thinking of more properties to expose for the widget, like being able to change the icon etc.
Attachment #638318 -
Flags: feedback?(dtownsend+bugmail) → feedback+
Comment 6•12 years ago
|
||
Comment on attachment 638318 [details] [diff] [review]
Prototype v1
(Already gave Blair feedback on this -- no issues -- forgot I was flagged)
Attachment #638318 -
Flags: feedback?(dolske) → feedback+
Reporter | ||
Comment 7•12 years ago
|
||
Going to start dumping regular work-in-progress patches here. Most obvious change here is getting the Panel and associated button looking like it fits in with Australis.
Attachment #638318 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Summary: Prototype/explore new Panel UI, and interactions with addons → New PanelUI and toolbar customization backend
Reporter | ||
Comment 8•12 years ago
|
||
This implements a decent amount of the desired functionality - a new app panel UI, integrates with hardcoded/overlaid toolbar buttons, integrates with in-content UI, provides an API for defining new widgets which are automatically handled for all windows, provides an API and events for a new customization mode to customize toolbars/the panel, stores/restores state (moving away from localstore.rdf), implements a large number of bugs, and maybe even does other things I've forgotten.
Note: Windows-only for now.
Attachment #658462 -
Attachment is obsolete: true
Reporter | ||
Comment 9•12 years ago
|
||
And this is an initial exploration into how the new customization mode would integrate with the rest of the browser - without actually getting into doing anything useful like customizing stuff.
Uses CSS transitions for the zoom-in animation when entering the customization mode - which works very smoothly on my desktop, but is very far from smooth on my Win8 Slate (which has much slower hardware). I haven't done any investigation into how to make that better on slow hardware, but at the moment the performance is unacceptable.
Note: This patch needs to be applied on top of the other patch. And it's also Windows-only.
Reporter | ||
Comment 10•12 years ago
|
||
And finally, this is an add-on that acts as a demo for UI customization, in lieu of a proper customization mode. It'll add a "UI Demo" button to the new panel - hitting that will open a tab with a craptastic UI to add/remove/reorganise items in the panel (changes are reflected immediately).
Obvious bug: Disabling/uninstalling this add-on currently won't remove it's button in the panel. That's a pending to-do item.
Comment 11•12 years ago
|
||
Try run for 4c6aade0dec7 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=4c6aade0dec7
Results (out of 1 total builds):
success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-4c6aade0dec7
Reporter | ||
Comment 12•12 years ago
|
||
In case it wasn't clear: The demo add-on will *only* work when using the above Try build (or any build with the attached patches applied).
Updated•12 years ago
|
Component: General → Toolbars
Comment 13•12 years ago
|
||
First off - this is really cool. I remember seeing shorlander / bwinton's mockup, and really digging it. It's cool to see this stuff get close.
Anyhow, a few things I noticed - not sure how you want these - as bugs, or a bullet list, but here goes:
* Clicking on menu button, then new window - menu stays on top and opened once new window has spawned. I expected that menu to close.
* The + and - buttons in the menu are not updating the text field between them.
* It's not clear to me what is customizable and what is not... I'm guessing dragging the toolbar buttons is currently not implemented? But if it is, it's currently busted.
Comment 14•12 years ago
|
||
Awesome guys, I'm following the progress of this feature and am very happy with the results.
I will send all the feedback that can help you.
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #13)
> * Clicking on menu button, then new window - menu stays on top and opened
> once new window has spawned. I expected that menu to close.
Yes, it should - consider that a bug. Seems to work for some things and not others.
> * The + and - buttons in the menu are not updating the text field between
> them.
Interesting, you expect that to reflect the current zoom level? The "100%" is intended to indicate that clicking it will reset the zoom to normal.
> * It's not clear to me what is customizable and what is not... I'm guessing
> dragging the toolbar buttons is currently not implemented? But if it is,
> it's currently busted.
Indeed, not implemented. The only way to do any customizing at the moment is via the demo add-on, and that only affects what's in the panel (and no dragging there either - all done via the 4 buttons).
(In reply to cuentaparafacultad from comment #14)
> I will send all the feedback that can help you.
Please do :)
Comment 16•12 years ago
|
||
Where can I download the try build ? It's not available.
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Darren Kalck [:D-Kalck] from comment #16)
> Where can I download the try build ? It's not available.
http://people.mozilla.org/~jwein/australis-customization.zip
Reporter | ||
Comment 18•12 years ago
|
||
General theme since previous iteration has been stabilization, formalization, bug fixes. ie, ripping out quick hacks and doing things properly :)
* Cleaner API
* Formalized registering of widgets, areas, and build instances
* Properly handling restoring of state
* Properly handling auto-adding widgets when they're first registered by an addon
* Start of standardizing widget wrappers (ie, even old-style XUL widgets can get nice wrappers now)
* Maybe other stuff I've forgotten
Also exploring some re-organization of the panel.
Attachment #672732 -
Attachment is obsolete: true
Reporter | ||
Comment 19•12 years ago
|
||
Just fixing bitrot.
Attachment #672734 -
Attachment is obsolete: true
Reporter | ||
Comment 20•12 years ago
|
||
More bitrot fixes (ie, you *need* to use this version of the demo addon).
Again, everything still Windows focused.
Attachment #672737 -
Attachment is obsolete: true
Reporter | ||
Comment 21•12 years ago
|
||
Er, helps if I attach the right patch.
Attachment #680574 -
Attachment is obsolete: true
Comment 22•12 years ago
|
||
Try run for 974fca53ce6a is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=974fca53ce6a
Results (out of 2 total builds):
success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-974fca53ce6a
Comment 23•12 years ago
|
||
Just one little detail on the current implementation : it would be more logical to have plus-minus zoom button inverted since we read LTR.
Reporter | ||
Comment 24•12 years ago
|
||
(In reply to Guillaume C. [:ge3k0s] from comment #23)
> Just one little detail on the current implementation : it would be more
> logical to have plus-minus zoom button inverted since we read LTR.
Indeed - fixed.
Comment 25•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #13)
> * The + and - buttons in the menu are not updating the text field between
> them.
I agree that this should be the default behaviour. The fact to click on this button could however always bring back 100% zoom.
Comment 26•12 years ago
|
||
Where can i download the latest try-builds? I test the build into "australis-customization" zip, but it freezes and doesn't let me do anything.
Thanks
Comment 27•12 years ago
|
||
(In reply to Alejandro from comment #26)
> Where can i download the latest try-builds? I test the build into
> "australis-customization" zip, but it freezes and doesn't let me do anything.
>
> Thanks
Sorry, the problem was Stylish. I have an style that hides the app button and that caused the problem.
Thanks anyway and sorry.
Assignee | ||
Comment 28•12 years ago
|
||
Assignee: bmcbride → jaws
Attachment #680577 -
Attachment is obsolete: true
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #680575 -
Attachment is obsolete: true
Reporter | ||
Comment 30•12 years ago
|
||
For those following this project, and wondering WTF is going on: I unexpectedly had to take quite a bit of time off (concussion), so things stalled. While I'm still getting back on my feet, Jared is able to put some time into this.
This is the latest code I had - I had been about to upload these as a new version of the patches, so IIRC there are quite a bit of changes. Mostly in the implementation of the customization mode, and changes to support that. They applied cleaning at the end of November, but will need rebasing now.
Attachment #705508 -
Attachment is obsolete: true
Reporter | ||
Comment 31•12 years ago
|
||
Attachment #705509 -
Attachment is obsolete: true
Comment 32•12 years ago
|
||
Anyone knows wich UX or Nightly version will come with this features? (Approximately). Is just to have an idea of when they're going to be landed.
Thanks
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #705686 -
Attachment is obsolete: true
Assignee | ||
Comment 34•12 years ago
|
||
Attachment #705687 -
Attachment is obsolete: true
Assignee | ||
Comment 35•12 years ago
|
||
Pushed the rebased patches to try server:
https://tbpl.mozilla.org/?tree=Try&rev=04e9583cb516
(In reply to Alejandro from comment #32)
> Anyone knows wich UX or Nightly version will come with this features?
> (Approximately). Is just to have an idea of when they're going to be landed.
Approximate goal for this feature is Nightly 22.
Assignee | ||
Updated•12 years ago
|
Summary: New PanelUI and toolbar customization backend → New PanelUI and toolbar customization
Assignee | ||
Comment 36•12 years ago
|
||
Rebased and fixed some small issues. Uploading this WIP patch here so mconley can split out the menu panel from the customization code.
Attachment #711495 -
Attachment is obsolete: true
Attachment #711496 -
Attachment is obsolete: true
Comment 37•12 years ago
|
||
Comment 38•12 years ago
|
||
Ok, I've split up WIP Patch v4.02 into two parts. I had to bring CustomizationUI.jsm over to the Menu patch, since it seems to be responsible for opening and managing the panel / button state. We might want to extract it out and put that work somewhere else, but this is a start.
I also had to de-bitrot v4.03 a little bit. These patches apply cleanly on top of UX from Feb 19 (840adae6b6e4).
Attachment #716660 -
Attachment is obsolete: true
Comment 39•12 years ago
|
||
Comment on attachment 717280 [details] [diff] [review]
AppMenu WIP Patch v4.03
Moving the menu work out to bug 844281.
Attachment #717280 -
Attachment is obsolete: true
Comment 40•12 years ago
|
||
Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
Attachment #717285 -
Attachment is obsolete: true
Updated•12 years ago
|
Flags: sec-review?(fbraun)
Comment 41•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #40)
> Created attachment 719573 [details] [diff] [review]
> Customization WIP Patch v4.04
>
> Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
I'm attempting to apply this patch to UX and failing; from looking at the patches there seem to be some references to PanelUI pieces that aren't in UX. am I missing something? Is there another patch somewhere I must apply first?
Assignee | ||
Comment 42•12 years ago
|
||
(In reply to Mark Goodwin [:mgoodwin] from comment #41)
> (In reply to Mike Conley (:mconley) from comment #40)
> > Created attachment 719573 [details] [diff] [review]
> > Customization WIP Patch v4.04
> >
> > Updated for latest UX (1dda6c9bc6a6 - Feb 28, 2013)
>
> I'm attempting to apply this patch to UX and failing; from looking at the
> patches there seem to be some references to PanelUI pieces that aren't in
> UX. am I missing something? Is there another patch somewhere I must apply
> first?
Current work is proceeding at https://tbpl.mozilla.org/?tree=Jamun. Please download the builds from there or use https://hg.mozilla.org/projects/jamun for source.
Assignee | ||
Comment 43•12 years ago
|
||
Attachment #680576 -
Attachment is obsolete: true
Attachment #719573 -
Attachment is obsolete: true
Attachment #728398 -
Flags: review?(bmcbride)
Assignee | ||
Comment 44•12 years ago
|
||
Attachment #728399 -
Flags: review?(bmcbride)
Assignee | ||
Updated•12 years ago
|
Attachment #728398 -
Attachment description: 9a2477c31b28.patch → 9a2477c31b28.patch (part 1)
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #728400 -
Flags: review?(jAwS)
Assignee | ||
Comment 46•12 years ago
|
||
Attachment #728401 -
Flags: review?(mconley)
Assignee | ||
Comment 47•12 years ago
|
||
Attachment #728403 -
Flags: review?(mconley)
Assignee | ||
Comment 48•12 years ago
|
||
Attachment #728404 -
Flags: review?(jAwS)
Assignee | ||
Updated•12 years ago
|
Attachment #728404 -
Attachment description: 6235f69f1647.patch → 6235f69f1647.patch (part 6)
Assignee | ||
Comment 49•12 years ago
|
||
Attachment #728405 -
Flags: review?(jAwS)
Assignee | ||
Updated•12 years ago
|
Attachment #728405 -
Attachment description: 53cf1ebc6e85.patch → 53cf1ebc6e85.patch (part 7)
Assignee | ||
Comment 50•12 years ago
|
||
Attachment #728407 -
Flags: review?(bmcbride)
Assignee | ||
Comment 51•12 years ago
|
||
Attachment #728409 -
Flags: review?(mconley)
Assignee | ||
Comment 52•12 years ago
|
||
Attachment #728411 -
Flags: review?(jAwS)
Assignee | ||
Comment 53•12 years ago
|
||
Attachment #728412 -
Flags: review?(jAwS)
Assignee | ||
Comment 54•12 years ago
|
||
Attachment #728414 -
Flags: review?(bmcbride)
Assignee | ||
Comment 55•12 years ago
|
||
Attachment #728415 -
Flags: review?(jAwS)
Assignee | ||
Updated•12 years ago
|
Attachment #728414 -
Attachment description: f0e94249a97a.patch (part 11) → f0e94249a97a.patch (part 12)
Assignee | ||
Comment 56•12 years ago
|
||
Attachment #728418 -
Flags: review?(mconley)
Assignee | ||
Comment 57•12 years ago
|
||
Attachment #728419 -
Flags: review?(jAwS)
Assignee | ||
Comment 58•12 years ago
|
||
Assignee | ||
Comment 59•12 years ago
|
||
Attachment #728425 -
Flags: review?(mconley)
Assignee | ||
Updated•12 years ago
|
Attachment #728420 -
Flags: review?(jAwS)
Assignee | ||
Comment 60•12 years ago
|
||
Attachment #728426 -
Flags: review?(mconley)
Assignee | ||
Comment 61•12 years ago
|
||
Attachment #728427 -
Flags: review?(bmcbride)
Assignee | ||
Comment 62•12 years ago
|
||
Attachment #728428 -
Flags: review?(jAwS)
Assignee | ||
Comment 63•12 years ago
|
||
Attachment #728430 -
Flags: review?(bmcbride)
Assignee | ||
Comment 64•12 years ago
|
||
Attachment #728432 -
Flags: review?(jAwS)
Assignee | ||
Comment 65•12 years ago
|
||
Attachment #728433 -
Flags: review?(jAwS)
Assignee | ||
Comment 66•12 years ago
|
||
Attachment #728435 -
Flags: review?(mconley)
Assignee | ||
Comment 67•12 years ago
|
||
This is a big rollup patch. Hopefully this will be easier to review and contain less obsoleted code. The parts to review can be split up by file for simplicity.
Blair, can you review the code changes in /browser/base/*
Mike, can you review the code changes in /browser/components/*, /browser/modules/*, /toolkit/mozapps/*
I'll review the changes in /browser/themes/*, toolkit/themes/*, /browser/locales/*
Attachment #728398 -
Attachment is obsolete: true
Attachment #728399 -
Attachment is obsolete: true
Attachment #728400 -
Attachment is obsolete: true
Attachment #728401 -
Attachment is obsolete: true
Attachment #728403 -
Attachment is obsolete: true
Attachment #728404 -
Attachment is obsolete: true
Attachment #728405 -
Attachment is obsolete: true
Attachment #728407 -
Attachment is obsolete: true
Attachment #728409 -
Attachment is obsolete: true
Attachment #728411 -
Attachment is obsolete: true
Attachment #728412 -
Attachment is obsolete: true
Attachment #728414 -
Attachment is obsolete: true
Attachment #728415 -
Attachment is obsolete: true
Attachment #728418 -
Attachment is obsolete: true
Attachment #728419 -
Attachment is obsolete: true
Attachment #728420 -
Attachment is obsolete: true
Attachment #728425 -
Attachment is obsolete: true
Attachment #728426 -
Attachment is obsolete: true
Attachment #728427 -
Attachment is obsolete: true
Attachment #728428 -
Attachment is obsolete: true
Attachment #728430 -
Attachment is obsolete: true
Attachment #728432 -
Attachment is obsolete: true
Attachment #728433 -
Attachment is obsolete: true
Attachment #728435 -
Attachment is obsolete: true
Attachment #728398 -
Flags: review?(bmcbride)
Attachment #728399 -
Flags: review?(bmcbride)
Attachment #728400 -
Flags: review?(jAwS)
Attachment #728401 -
Flags: review?(mconley)
Attachment #728403 -
Flags: review?(mconley)
Attachment #728404 -
Flags: review?(jAwS)
Attachment #728405 -
Flags: review?(jAwS)
Attachment #728407 -
Flags: review?(bmcbride)
Attachment #728409 -
Flags: review?(mconley)
Attachment #728411 -
Flags: review?(jAwS)
Attachment #728412 -
Flags: review?(jAwS)
Attachment #728414 -
Flags: review?(bmcbride)
Attachment #728415 -
Flags: review?(jAwS)
Attachment #728418 -
Flags: review?(mconley)
Attachment #728419 -
Flags: review?(jAwS)
Attachment #728420 -
Flags: review?(jAwS)
Attachment #728425 -
Flags: review?(mconley)
Attachment #728426 -
Flags: review?(mconley)
Attachment #728427 -
Flags: review?(bmcbride)
Attachment #728428 -
Flags: review?(jAwS)
Attachment #728430 -
Flags: review?(bmcbride)
Attachment #728432 -
Flags: review?(jAwS)
Attachment #728433 -
Flags: review?(jAwS)
Attachment #728435 -
Flags: review?(mconley)
Attachment #728439 -
Flags: review?(mconley)
Attachment #728439 -
Flags: review?(jAwS)
Attachment #728439 -
Flags: review?(bmcbride)
Comment 68•12 years ago
|
||
Can someone explain how this is supposed to work? It looks quite a bit different to what I expected from the Feature page on https://wiki.mozilla.org/Features/Desktop/Panel_Menu
Comment 69•12 years ago
|
||
(In reply to Frederik Braun [:freddyb] from comment #68)
> Can someone explain how this is supposed to work? It looks quite a bit
> different to what I expected from the Feature page on
> https://wiki.mozilla.org/Features/Desktop/Panel_Menu
We're attacking this bug iteratively, and this is our first iteration. We'll review this, and generate patches on top of it, and review those, etc.
So yes, it will look a bit different from https://wiki.mozilla.org/Features/Desktop/Panel_Menu - we haven't quite gotten there yet.
Assignee | ||
Comment 70•12 years ago
|
||
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch
Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------
I think we can undo the changes to syncProgress.css and all three of the inContentUI.css files. They aren't necessary for this first part, and will make the impact more focused.
I'll put together a patch to fix the issues that I have found and request review for it.
::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +14,5 @@
> <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
> inside the private browsing mode -->
> <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
>
> +<!ENTITY appmenu.title "&brandFullName; menu">
"Customize and Control &brandFullName;"
::: browser/themes/linux/panelUIOverlay.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +%filter substitution
> +
> +%define menuPanelWidth 23em
Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a blank line between the %define and the %filter, but in windows/panelUIOverlay.css the %define is immediately below the %filter. We should make all these files the same.
Also, they all have the same menuPanelWidth. Should that get moved to the shared file?
::: browser/themes/shared/panelUIOverlay.inc.css
@@ +77,5 @@
> +}
> +
> +#PanelUI-subViews {
> + -moz-stack-sizing: ignore;
> + background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
This and other places should use the unprefixed syntax. Note that the we can't just drop the -moz- to fix this.
@@ +79,5 @@
> +#PanelUI-subViews {
> + -moz-stack-sizing: ignore;
> + background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
> + background-color: -moz-dialog;
> + box-shadow: -1px 0px 0px rgba(0, 0, 0, 0.2), -1px 0px 2px rgba(0, 0, 0, 0.1), 1px 0px 0px rgba(255, 255, 255, 0.2) inset; /* stolen from shorlander's mock-up */
Probably can remove the "stolen from shorlander's mockup" comment.
@@ +206,5 @@
> +#customization-palette .toolbarbutton-text {
> + display: none;
> +}
> +
> +/* XXXunf Hack! Copied from general toolbar styles in winstripe. */
This styling will be used for all platforms. Should the windows theme reference this? Or should they just be duplicated since they serve different purposes and they may intentionally diverge over time? I'm fine with the duplication.
@@ +240,5 @@
> + background-color: hsla(210,54%,20%,.15);
> + border-color: hsla(210,54%,20%,.3) hsla(210,54%,20%,.35) hsla(210,54%,20%,.4);
> + box-shadow: 0 1px 1px hsla(210,54%,20%,.1) inset,
> + 0 0 1px hsla(210,54%,20%,.2) inset,
> + /* allows winstripe-keyhole-forward-clip-path to be used for non-hover as well as hover: */
This comment doesn't apply here.
::: browser/themes/windows/browser.css
@@ +2805,5 @@
> +
> +
> +
> +
> +/* Woo! Customization Mode! YEA!!!! */
We can remove this comment now, and move this code closer to the above section.
::: browser/themes/windows/jar.mn
@@ +19,5 @@
> #ifdef MOZ_SERVICES_SYNC
> skin/classic/browser/aboutSyncTabs.css
> #endif
> skin/classic/browser/actionicon-tab.png
> + skin/classic/aero/browser/appmenu.png (appmenu.png)
This is in the wrong section, it should be down with the other aero files. Also, the rename here (appmenu.png) is unnecessary since it's the same as the original filename.
We also need the appmenu.png images for linux and osx.
Attachment #728439 -
Flags: review?(jAwS) → review+
Comment 71•12 years ago
|
||
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch
Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------
So, I had a sweet 2 hour review, and then Bugzilla ate it. :/ Doing another one now, hopefully it's as cool as my last one. *sigh*
I'll review CustomizeMode.jsm next.
::: browser/components/CustomizableUIMediator.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
Needs documentation describing what this service is responsible for.
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
Trailing whitespace and busted alignment on line 13.
@@ +13,5 @@
> + "resource:///modules/CustomizableUI.jsm");
> +
> +
> +function CustomizableUIMediator() {
> + this.wrappedJSObject = this;
Does any caller ever use the wrappedJSObject? If not, we should probably axe it.
::: browser/modules/CustomizableUI.jsm
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +
> +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
As per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style, consts should be k prefixed and camelcased, like:
const kNSXUL = ...;
const kPrefCustomizationState = ...;
etc.
@@ +16,5 @@
> +const PREF_CUSTOMIZATION_STATE = "browser.uiCustomization.state";
> +const PREF_CUSTOMIZATION_AUTOADD = "browser.uiCustomization.autoAdd";
> +const PREF_CUSTOMIZATION_DEBUG = "browser.uiCustomization.debug";
> +
> +const kEventHandlers = {
This name is too generic. We either need a better name, or some documentation here, or both.
@@ +28,5 @@
> + */
> +let gPalette = new Map();
> +
> +/**
> + * gAreas maps area IDs to properties about those areas. An area is a place
Sets of properties might be more accurate.
@@ +31,5 @@
> +/**
> + * gAreas maps area IDs to properties about those areas. An area is a place
> + * where a widget can be put.
> + */
> +let gAreas = new Map();
Blair: I switched from using generic area names to using IDs for the area nodes. I didn't see any advantage to another layer of abstraction - but I could be wrong... any objections?
@@ +101,5 @@
> + gDebug = Services.prefs.getBoolPref(PREF_CUSTOMIZATION_DEBUG);
> +} catch (e) {}
> +
> +function LOG(aMsg) {
> + if (gDebug)
Throughout both CustomizableUI.jsm and CustomizeMode.jsm, I see a lot of no-braces-for-one-liners. According to https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style, we need to brace these - so this is just a general issue for both files.
@@ +119,5 @@
> + this.registerArea(CustomizableUI.AREA_PANEL);
> + this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> + },
> +
> + _defineBuiltInWidgets: function() {
A general note - all of these functions require documentation.
@@ +120,5 @@
> + this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> + },
> +
> + _defineBuiltInWidgets: function() {
> + //XXXunf Need a better place to define these.
Is this a better-enough place?
@@ +253,5 @@
> + },
> +
> + registerArea: function(aName, aProperties) {
> + if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> + throw Error("Invalid area name");
throw new Error
@@ +255,5 @@
> + registerArea: function(aName, aProperties) {
> + if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> + throw Error("Invalid area name");
> + if (gAreas.has(aName))
> + throw Error("Area already registered");
throw new Error
@@ +274,5 @@
> + let document = aToolbar.ownerDocument;
> + let area = aToolbar.id;
> +
> + if (!gAreas.has(area))
> + throw Error("Unknown customization area");
throw new Error
@@ +286,5 @@
> + let legacyState = aToolbar.getAttribute("currentset");
> + if (legacyState)
> + legacyState = legacyState.split(",");
> + //XXXunf should the legacy attribute be purged?
> + // kinda messes up switching to older builds
I think we should do best-effort to maintain currentset, until this new customization stuff is shipped, and then I think we should drop currentset like a hot rock.
@@ +320,5 @@
> + if (provider == CustomizableUI.PROVIDER_XUL && aArea == CustomizableUI.AREA_PANEL)
> + this.ensureButtonClosesPanel(node);
> +
> + container.insertBefore(node, currentNode);
> + currentNode = node.nextSibling;
Since node was just inserted before currnetNode, node.nextSibling is already currentNode, so I don't think we need line 324.
@@ +328,5 @@
> + let palette = aAreaNode.toolbox ? aAreaNode.toolbox.palette : null;
> + let limit = currentNode.previousSibling;
> + let node = container.lastChild;
> + while (node != limit) {
> + // XXXunf Depreciating the old "removable" attribute, is this right?
Not sure. The Social API button, for example, isn't supposed to be move-able. I'm not even sure how that is supposed to play in this new Australis customization world we're building here. Worth investigating.
@@ +355,5 @@
> + if (this.isSpecialWidget(aWidgetId))
> + return CustomizableUI.PROVIDER_SPECIAL;
> + if (gPalette.has(aWidgetId))
> + return CustomizableUI.PROVIDER_API;
> + //XXXunf Er, this isn't entirely true.. could also not exist.
We should handle that case, and log or throw as necessary.
@@ +393,5 @@
> +
> + aPanel.toolbox = document.getElementById("navigator-toolbox");
> + aPanel.customizationTarget = panelContents;
> +
> + //XXXjaws Not sure if this is correct, the build area while in customization mode is actually the panelContents.
This has been corrected - this comment can be removed.
@@ +398,5 @@
> + let placements = gPlacements.get(CustomizableUI.AREA_PANEL);
> + this.buildArea(CustomizableUI.AREA_PANEL, placements, aPanel);
> + this.registerBuildArea(CustomizableUI.AREA_PANEL, aPanel);
> +
> + // XXXmconley: not sure if registering the panel is when we'd want to
I still think this is true. I think windows on load should register with CustomizableUI, so that when they unload, CustomizableUI can clean up.
@@ +451,5 @@
> + if (!areaNodes)
> + return;
> +
> + for (let areaNode of areaNodes) {
> + let container = areaNode.customizationTarget;
Can we assume that every area node has a customizationTarget? If not, we should check for it when they register.
@@ +544,5 @@
> +
> + if (aToolbox.palette) {
> + // Attempt to locate an node with a matching ID within
> + // the palette.
> + for (let node of aToolbox.palette.children) {
I to believe we could just use aToolbox.palette.querySelector("#" + aId) here.
@@ +552,5 @@
> + }
> + return null;
> + },
> +
> + //XXXunf Need to free this from aMenu's tyranny.
Not sure what this comment means, though I do appreciate the use of the word tyranny. :) This should either be made more clear, or just be removed.
@@ +557,5 @@
> + buildWidget: function(aDocument, aMenu, aWidget) {
> + if (typeof aWidget == "string")
> + aWidget = gPalette.get(aWidget);
> + if (!aWidget)
> + return null;
We should log / throw here, instead of just returning null.
@@ +621,5 @@
> + } catch (e) {
> + Cu.reportError(e);
> + }
> + } else {
> + //XXXunf Need to think this through more, and formalize.
Blair: What scenario is this else for?
@@ +668,5 @@
> + if (!this.getPlacementOfWidget(node.id))
> + widgets.add(node.id);
> + }
> +
> + // XXXmconley: is there any good reason why we're not just returning
As far as I can tell, the answer is no.
@@ +675,5 @@
> + },
> +
> + getPlacementOfWidget: function(aWidgetId) {
> + let widget = gPalette.get(aWidgetId);
> + //XXXunf Pretty sure this won't work anymore, thanks to restoring widget
registerWidget isn't a function anymore, and restoring widget placement is handled elsewhere. I think this block can go.
@@ +694,5 @@
> + },
> +
> + addWidgetToArea: function(aWidgetId, aArea, aPosition) {
> + if (!gAreas.has(aArea))
> + throw Error("Unknown customization area");
throw new Error
@@ +913,5 @@
> + if (aForceSave === true)
> + gDirty = true;
> + this.saveState();
> + },
> + //XXXunf Used only for conveience when developing stuff, need to remove this.
This function is no longer used and can be removed.
@@ +1059,5 @@
> +
> + if (typeof aData.id != "string" || !/^[a-z0-9-_]{1,}$/i.test(aData.id))
> + return null;
> +
> + const REQ_STRING_PROPS = ["id", "name"];
consts should be k prefixed and camel-cased.
@@ +1099,5 @@
> + aData.onCommand :
> + null;
> + } else if (widget.type == "view") {
> + if (typeof aData.viewId != "string")
> + return null;
Should log / throw here.
@@ +1147,5 @@
> + this.notifyListeners("onWidgetDestroyed", aWidgetId);
> + },
> +
> + registerManifest: function(aBaseLocation, aDirective) {
> + let tokens = aData.split(/\s+/);
aData is not defined - did you mean aDirective?
@@ +1310,5 @@
> + );
> + },
> + getWidgetsInArea: function(aArea) {
> + if (!gAreas.has(aArea))
> + throw Error("Unknown customization area");
throw new Error
@@ +1312,5 @@
> + getWidgetsInArea: function(aArea) {
> + if (!gAreas.has(aArea))
> + throw Error("Unknown customization area");
> + if (!gPlacements.has(aArea))
> + throw Error("Area not yet restored");
throw new Error
@@ +1322,5 @@
> + },
> + get areas() {
> + return [area for ([area, props] of gAreas)];
> + },
> + getCustomizeTargetsForArea: function(aArea) {
This is getting a bit complex. The different customize targets for an area are the different instances of that area across each browser window. We must make sure that we don't give the impression that an area can have multiple customize targets. I do know that UX wants to have, for example, multiple targets in the main panel...those should be individual areas, I think.
@@ +1333,5 @@
> +Object.freeze(this.CustomizableUI);
> +
> +
> +
> +function WidgetGroupWrapper(aWidget) {
The purpose and function of these wrappers isn't 100% clear to me. I think we need some documentation here.
Comment 72•12 years ago
|
||
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch
Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/CustomizeMode.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["CustomizeMode"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +const DEBUG = true;
kDebug, as per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style. Should default to false.
@@ +33,5 @@
> + // areas and added to the palette, they're added to the visible palette.
> + // _stowedPalette is a reference to the old invisible palette so we can
> + // restore gNavToolbox.palette to its original state after exiting
> + // customization mode.
> + this._stowedPalette = null;
Might as well just toss this into the prototype.
@@ +39,5 @@
> +
> +CustomizeMode.prototype = {
> + window: null,
> + document: null,
> + palette: null,
window and document are, imo, self explanatory. palette and areas should have some documentation as to their purpose.
@@ +42,5 @@
> + document: null,
> + palette: null,
> + areas: null,
> +
> + enter: function() {
Each of these functions requires documentation, and newlines between.
@@ +50,5 @@
> + let document = this.document;
> +
> + // Disable the toolbar context menu items
> + let menubar = document.getElementById("main-menubar");
> + for (let childNode of menubar.childNodes)
So I think what might be cleaner is if we fire a "customize start" event, and have code in browser.js listen for and react to that event to "batten down the hatches" for customization mode. This is also more extensible.
@@ +89,5 @@
> +
> + // Add drag-and-drop event handlers to all of the customizable areas.
> + self.areas = [];
> + for (let area of CustomizableUI.areas) {
> + let targets = CustomizableUI.getCustomizeTargetsForArea(area);
This should just get the area instance for this particular window.
@@ +118,5 @@
> + CustomizableUI.removeListener(this);
> +
> + this.depopulatePalette();
> +
> + this.visiblePalette.removeEventListener("dragstart", this._onDragStart);
These won't remove correctly, since the function signatures don't match with when we added the listeners.
@@ +131,5 @@
> + for (let target of this.areas) {
> + for (let toolbarItem of target.children) {
> + this.unwrapToolbarItem(toolbarItem);
> + }
> + target.removeEventListener("dragstart", this._onDragStart);
Same as above - these won't remove correctly.
@@ +142,5 @@
> + let browser = document.getElementById("browser");
> + browser.parentNode.selectedPanel = browser;
> +
> + // Update global UI elements that may have been added or removed
> + if (aToolboxChanged) {
aToolboxChanged is *never* true, so this block is never executed.
As in enter, I think we should fire a "customization done" event when completed, and then have browser.js react to it to get us back to normal browsing. I also think we should track whether or not customization took place, and pass a "aToolboxChanged" equivalent bool to the event listeners.
@@ +168,5 @@
> + // to old builds. We might want to keep this around for a little
> + // bit.
> + this.persistCurrentSets();
> + } else {
> + LOG("Exiting customize mode with no changes.");
This message isn't necessarily true, and should be removed.
@@ +212,5 @@
> + populatePalette: function() {
> + let toolboxPalette = this.window.gNavToolbox.palette;
> +
> + let unusedWidgets = CustomizableUI.getUnusedWidgets(toolboxPalette);
> + LOG("List of unused widgets: " + unusedWidgets);
This log message just outputs [Object, Object, Object...]. Not useful, should be removed.
@@ +215,5 @@
> + let unusedWidgets = CustomizableUI.getUnusedWidgets(toolboxPalette);
> + LOG("List of unused widgets: " + unusedWidgets);
> + for (let widget of unusedWidgets) {
> + let paletteItem = this.makePaletteItem(widget, "palette");
> + paletteItem.setAttribute("style", "display: inline-block; -moz-orient: vertical; padding: 1em;"); //XXXunf Hack!
Hack indeed - this should be taken care of in CSS.
@@ +222,5 @@
> +
> + this._stowedPalette = this.window.gNavToolbox.palette;
> + this.window.gNavToolbox.palette = this.visiblePalette;
> + },
> + //XXXunf Maybe this should use -moz-element instead of wrapping the node?
I'm not opposed to trying this - makes more sense to me to "imitate"/"mimic" the toolbaritems rather than wrapping them. Also means we don't have to slurp the items in and out of the invisible palette each time.
Might be worth investigating later.
@@ +259,5 @@
> + }
> +
> + this._stowedPalette.appendChild(widgetNode);
> + } else if (provider = CustomizableUI.PROVIDER_API) {
> + //XXXunf Currently this doesn't destroy the (now unused) node. It would
Let's just leave this for now, and deal with it in a later iteration.
@@ +281,5 @@
> + },
> + createWrapper: function(aNode, aPlace) {
> + let wrapper = this.document.createElement("toolbarpaletteitem");
> +
> + //XXXjaws Is this attribute needed?
This conversation should be replaced with an explanation of what "place" is for.
@@ +344,5 @@
> + //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> + // also want to (try) persisting with currentset as well to make it
> + // less painful to switch to older builds.
> + persistCurrentSets: function() {
> + //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)
jaws is right - we should try to fix the binding to inherit from toolkit toolbar.
@@ +352,5 @@
> + let document = this.document;
> + let toolbar = document.getElementById("nav-bar");
> +
> + // Calculate currentset and store it in the attribute.
> + var currentSet = toolbar.currentSet;
Use let, not var.
@@ +374,5 @@
> + },
> + onWidgetDestroyed: function(aWidgetId) {
> + },
> +
> + _onDragStart: function(aEvent) {
In the following functions, I see a lot of "var" usage. Switch these to "let".
@@ +383,5 @@
> + return;
> + item = item.parentNode;
> + }
> +
> + item.setAttribute("dragactive", "true");
What is this attribute used for?
@@ +415,5 @@
> + let targetParent = targetNode.parentNode;
> + let targetArea = this._getCustomizableParent(targetNode);
> + let originArea = this._getCustomizableParent(draggedWrapper);
> +
> + // Do nothing if the target is not the panel or the palette, or if the
Comment is inaccurate - do nothing if the targetArea or originArea aren't customizable.
@@ +468,5 @@
> + let itemFlex = parseInt(draggedWrapper.getAttribute("flex"), 10);
> + parent.setAttribute("flex", parentFlex + itemFlex);
> + }
> +
> + LOG("Done!");
Not a very useful log message. Probably safe to remove.
Attachment #728439 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 73•12 years ago
|
||
Attachment #729145 -
Flags: review?(mconley)
Assignee | ||
Updated•12 years ago
|
Attachment #729145 -
Attachment is obsolete: true
Attachment #729145 -
Flags: review?(mconley)
Assignee | ||
Comment 74•12 years ago
|
||
Attachment #729180 -
Flags: review?(mconley)
Comment 75•12 years ago
|
||
Updated•12 years ago
|
Attachment #729248 -
Flags: review?(jAwS)
Comment 76•12 years ago
|
||
Hey Blair,
Is it possible for you to review your section sometime today? Jared and I would like to land a version of this patch on UX sometime on March 26th.
Thanks,
-Mike
Flags: needinfo?(bmcbride)
Comment 77•12 years ago
|
||
Comment on attachment 729180 [details] [diff] [review]
Addressing my review comments
Review of attachment 729180 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me. Thanks Jared!
::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +14,5 @@
> <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
> inside the private browsing mode -->
> <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
>
> +<!ENTITY appmenu.title "Customize and Control &brandFullName;">
I'm assuming this was part of a spec somewhere.
::: browser/themes/windows/browser.css
@@ +2841,5 @@
> #customization-panelHolder > #PanelUI-mainView {
> box-shadow: inset 0 0 3px blue;
> border-radius: 2px;
> }
> +/* end Customization mode */
I added a rule here in my fixup patch, so we're going to conflict here. No biggie.
Attachment #729180 -
Flags: review?(mconley) → review+
Comment 78•12 years ago
|
||
Re-based on top of jaws' fixes.
Attachment #729248 -
Attachment is obsolete: true
Attachment #729248 -
Flags: review?(jAwS)
Attachment #729253 -
Flags: review?(jAwS)
Assignee | ||
Comment 79•12 years ago
|
||
Comment on attachment 729248 [details] [diff] [review]
Addressing my review comments as well
Review of attachment 729248 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/CustomizableUI.jsm
@@ +337,5 @@
> continue;
> }
>
> + if (provider == CustomizableUI.PROVIDER_XUL
> + && aArea == CustomizableUI.AREA_PANEL) {
nit: && should go at the end of the first line, like so:
if (provider == CustomizableUI.PROVIDER_XUL &&
aArea == CustomizableUI.AREA_PANEL) {
...
}
here, and lower in the file too.
::: browser/modules/CustomizeMode.jsm
@@ +115,5 @@
>
> let window = this.window;
> let document = this.document;
>
> + // Let everybody in this window know that we're about to customize.
// Let everybody in this window know that we're about to finish customizing.
Attachment #729248 -
Attachment is obsolete: false
Assignee | ||
Comment 80•12 years ago
|
||
Comment on attachment 729253 [details] [diff] [review]
Addressing my review comments (apply on top of jaws' patch)
Review of attachment 729253 [details] [diff] [review]:
-----------------------------------------------------------------
See feedback from previous review.
Attachment #729253 -
Flags: review?(jAwS) → review+
Updated•12 years ago
|
Attachment #729248 -
Attachment is obsolete: true
Comment 81•12 years ago
|
||
So, the UX team had some further suggestions / requests for us to get in before we try merging this stuff over to the UX branch. This is part #1 of #9 for these fixes.
Don't worry - these fixes are pretty small.
The big roll-up patch should have these 9 patches applied on top of it once it has full r+.
Comment 82•12 years ago
|
||
Before, we only appended to the palette. This allows us to drop amongst the palette items.
Comment 83•12 years ago
|
||
Programmatically generated widgets wouldn't behave properly after dragging / dropping them once. After dragging or dropping them, they'd act as their unwrapped selves.
Comment 84•12 years ago
|
||
This patch replace the flat blue background with the grid texture, which I cribbed from the shorlander / bwinton mock-up. It's a JPG for now, so we're going to want to get this replaced.
Comment 85•12 years ago
|
||
This patch allows us to exit customize mode either by pressing Esc, or by clicking on the menu panel button.
Comment 86•12 years ago
|
||
This adds a very basic (and temporary) drop indicator to the nav-bar when customizing.
Comment 87•12 years ago
|
||
Our drop targets had a purplish hue about them. This makes them more blue.
Comment 88•12 years ago
|
||
The menu button should be in the disabled state while in customizing mode.
Comment 89•12 years ago
|
||
When "picking up" a customizable item, we scale it up and increase its opacity.
Comment 90•12 years ago
|
||
So, just to give a sense of review priority: the big base-line patch is absolutely top priority.
The After-big-patch Follow-ups come after.
Updated•12 years ago
|
Attachment #729293 -
Flags: review?(mconley)
Updated•12 years ago
|
Attachment #729287 -
Flags: review?(mconley)
Updated•12 years ago
|
Attachment #729295 -
Flags: review?(mconley)
Updated•12 years ago
|
Attachment #729289 -
Flags: review?(bmcbride)
Updated•12 years ago
|
Attachment #729290 -
Flags: review?(bmcbride)
Updated•12 years ago
|
Attachment #729291 -
Flags: review?(bmcbride)
Updated•12 years ago
|
Attachment #729292 -
Flags: review?(jAwS)
Updated•12 years ago
|
Attachment #729294 -
Flags: review?(jAwS)
Updated•12 years ago
|
Attachment #729296 -
Flags: review?(jAwS)
Comment 91•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #71)
> > +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>
> As per
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style,
> consts should be k prefixed and camelcased, like:
No, there's no such rule. That document just explains what k means when it's used. It correctly points out that we don't use that style across the code base.
> > + registerArea: function(aName, aProperties) {
> > + if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > + throw Error("Invalid area name");
>
> throw new Error
Why?
Comment 92•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #91)
> (In reply to Mike Conley (:mconley) from comment #71)
> > > +const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> >
> > As per
> > https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style,
> > consts should be k prefixed and camelcased, like:
>
> No, there's no such rule. That document just explains what k means when it's
> used. It correctly points out that we don't use that style across the code
> base.
Ah, true. You're right. Didn't realize we had a spectrum of styles for constants in Javascript.
>
> > > + registerArea: function(aName, aProperties) {
> > > + if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > > + throw Error("Invalid area name");
> >
> > throw new Error
>
> Why?
Because I saw both "throw new Error" and "throw Error" being used, and I opted for visual consistency. Functionally, I believe they're equivalent.
Updated•12 years ago
|
Attachment #729287 -
Flags: review?(mconley) → review+
Comment 93•12 years ago
|
||
Comment on attachment 729293 [details] [diff] [review]
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar
Review of attachment 729293 [details] [diff] [review]:
-----------------------------------------------------------------
Just a few minor suggestions, but otherwise, this looks good to me.
::: browser/modules/CustomizeMode.jsm
@@ +105,5 @@
> this._onDragStart.bind(this));
> this.visiblePalette.addEventListener("dragover",
> this._onDragOver.bind(this));
> + this.visiblePalette.addEventListener("dragexit",
> + this._onDragOver.bind(this));
Nope, this is wrong - this should be this._onDragExit.bind(this).
Probably my fault when de-bitrotting these.
Code review saves the day again!
@@ +409,5 @@
> + let targetParent = targetNode.parentNode;
> + let targetArea = this._getCustomizableParent(targetNode);
> + let originArea = this._getCustomizableParent(draggedWrapper);
> +
> + // Do nothing if the target is not the panel or the palette, or if the
Comment should be, "Do nothing if the target or origin is not customizable"
@@ +421,5 @@
> + // the target.
> + let position = Array.indexOf(targetParent.children, targetNode);
> + let dragOverItem = position == -1 ? targetParent.lastChild : targetParent.children[position];
> +
> + if (this._dragOverItem && dragOverItem != this._dragOverItem)
Brace the one-liner.
@@ +508,5 @@
> }
> },
>
> + _onDragExit: function(aEvent) {
> + if (this._dragOverItem)
Brace the one-liner
@@ +514,5 @@
> + },
> +
> + // XXXjaws Show a ghost image or blank area where the item could be added, instead of black bar
> + _setDragActive: function(aItem, aValue) {
> + var node = aItem;
These var's should be let's.
@@ +523,5 @@
> + node = aItem.lastChild;
> + value = direction == "ltr"? "right" : "left";
> + }
> +
> + if (!node)
Brace this one-liner
@@ +527,5 @@
> + if (!node)
> + return;
> +
> + if (aValue) {
> + if (!node.hasAttribute("dragover"))
Brace this one-liner
@@ +560,5 @@
> +function getPlaceForItem(aElement) {
> + let place;
> + let node = aElement;
> + while (node && !place) {
> + if (node.localName == "toolbar")
Brace the one-liners in here.
@@ +562,5 @@
> + let node = aElement;
> + while (node && !place) {
> + if (node.localName == "toolbar")
> + place = "toolbar";
> + else if (node.id == "PanelUI-contents")
Instead of hard-coding this ID, I think we should use CustomizableUI.AREA_PANEL
Attachment #729293 -
Flags: review?(mconley) → review+
Updated•12 years ago
|
Attachment #729295 -
Flags: review?(mconley) → review+
Reporter | ||
Comment 94•12 years ago
|
||
Comment on attachment 729253 [details] [diff] [review]
Addressing my review comments (apply on top of jaws' patch)
Review of attachment 729253 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/CustomizableUIMediator.js
@@ +5,5 @@
> +/**
> + * This component is responsible for catching unhandled directive events from
> + * the chrome manifest parser, and sending those off to the CustomizableUI
> + * module to see if the unhandled directives are add-ons registering
> + * customizable widgets.
This whole file should just be removed for now, given bug 793093 was WONTFIXed. No point in having this until we know what alternate API (if any) we'll have.
::: browser/modules/CustomizableUI.jsm
@@ +21,5 @@
> + * The keys are the handlers that are fired when the event type (the value)
> + * is fired on the subview. A widget that provides a subview has the option
> + * of providing onViewShowing and onViewHiding event handlers.
> + */
> +const kSubviewEventHandlers = {
Hardcoding both handler and event seems error-prone - given the handler name is always going to be consistent, "onEventName" for an event named "EventName".
@@ +302,4 @@
> // kinda messes up switching to older builds
> + //XXXmconley No, I don't think so - I think we want to make it easy to
> + // switch back and forth between builds until this thing hits
> + // release.
Yea, that was more of a long-term thought.
@@ +382,5 @@
> return CustomizableUI.PROVIDER_API;
> + }
> +
> + // It's also possible that this widget doesn't exist (yet). In either case,
> + // we fall back to the XUL provider.
My originally comment here meant that we fall back to the XUL provider, but we don't know for sure (at this point) whether it exists there either. So the API is technically lying. Ideally, it would be able to return an error value (or throw an exception) if it really didn't exist. Our code calling this function handles that fine, but this is a public API.
@@ +591,1 @@
> buildWidget: function(aDocument, aMenu, aWidget) {
Re-add my to-do comment here (re-worded)? Depending on aMenu here is a hack that should be fixed.
@@ +1109,5 @@
> icons: {}
> };
>
> + if (typeof aData.id != "string" || !/^[a-z0-9-_]{1,}$/i.test(aData.id)) {
> + throw new Error("Given an illegal id in normalizeWidget: " + aData.id);
If this is throwing an exception now, then a missing required property should do so too (and the case where it already exists). Also need to make sure the exception gets safely propagated to the outside caller, and doesn't break everything if this is during startup (because callers currently assume this doesn't throw).
::: browser/modules/CustomizeMode.jsm
@@ +7,5 @@
> this.EXPORTED_SYMBOLS = ["CustomizeMode"];
>
> const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>
> +const kDebug = true;
This should be changed to use the browser.uiCustomization.debug pref at some stage.
@@ +110,5 @@
> + this._onDragStart.bind(this));
> + this.visiblePalette.removeEventListener("dragover",
> + this._onDragOver.bind(this));
> + this.visiblePalette.removeEventListener("drop",
> + this._onDragDrop.bind(this));
Fun fact: this.someFunction.bind(this) !== this.someFunction.bind(this)
Therefore, removing event listeners like this doesn't work, because you're trying to remove a different listener :( Need to store the value you originally pass into addEventListener() if you want to use bind().
Same issue later in this function too.
@@ +115,5 @@
>
> let window = this.window;
> let document = this.document;
>
> + // Let everybody in this window know that we're about to customize.
This comment is inaccurate.
Flags: needinfo?(bmcbride)
Reporter | ||
Comment 95•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #71)
> Blair: I switched from using generic area names to using IDs for the area
> nodes. I didn't see any advantage to another layer of abstraction - but I
> could be wrong... any objections?
I think that should be fine. IIRC, it was just cosmetic.
Reporter | ||
Comment 96•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #71)
> @@ +621,5 @@
> > + } catch (e) {
> > + Cu.reportError(e);
> > + }
> > + } else {
> > + //XXXunf Need to think this through more, and formalize.
>
> Blair: What scenario is this else for?
That was for when the widget was registered via chrome.manifest - since that's more declarative, you generally want to avoid including code in the definition. And the JSON parser doesn't support code anyway (can work around that, if need be). So in lieu of a callable onCommand property, it would send some kind of event/notification that the add-on would listen for.
Assignee | ||
Comment 97•12 years ago
|
||
Comment on attachment 729292 [details] [diff] [review]
After-big-patch Follow-up #5 - More ways to exit customize mode
Review of attachment 729292 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/CustomizeMode.jsm
@@ +361,5 @@
> + this.exit();
> + }
> + break;
> + case "click":
> + if (aEvent.originalTarget = this.window.PanelUI.menuButton) {
This should also check aEvent.button == 0.
Attachment #729292 -
Flags: review?(jAwS) → review+
Assignee | ||
Comment 98•12 years ago
|
||
Comment on attachment 729293 [details] [diff] [review]
After-big-patch Follow-up #6 - Add rudimentary drop indicator to toolbar
Review of attachment 729293 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/CustomizeMode.jsm
@@ +126,5 @@
> this._onDragStart.bind(this));
> this.visiblePalette.removeEventListener("dragover",
> this._onDragOver.bind(this));
> + this.visiblePalette.removeEventListener("dragexit",
> + this._onDragExit.bind(this));
Calling .bind() like this for add/removeEventListener is bad since it's not actually removing the event listener that was added before. Each call to bind() generates a new function, so calling removeEventListener with fn.bind isn't going to remove the original event listener. If we need to use .bind(), then we need to cache the result of .bind() and pass that return value to both add/removeEventListener.
Attachment #729293 -
Flags: review-
Assignee | ||
Updated•12 years ago
|
Attachment #729294 -
Flags: review?(jAwS) → review+
Reporter | ||
Comment 99•12 years ago
|
||
Comment on attachment 729289 [details] [diff] [review]
After-big-patch Follow-up #2 - Allow items to be dropped among palette items
Review of attachment 729289 [details] [diff] [review]:
-----------------------------------------------------------------
... even though the next patch rewrites the only change here...
Attachment #729289 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 100•12 years ago
|
||
Comment on attachment 729296 [details] [diff] [review]
After-big-patch Follow-up #9 - Add a grab effect when drag starts
Review of attachment 729296 [details] [diff] [review]:
-----------------------------------------------------------------
r- for the event listener leak.
::: browser/modules/CustomizeMode.jsm
@@ +290,5 @@
> wrapper.setAttribute("flex", aNode.getAttribute("flex"));
> }
>
> + wrapper.addEventListener("mousedown", this._onMouseDown.bind(this));
> + wrapper.addEventListener("mouseup", this._onMouseUp.bind(this));
Same comment here as in previous review, we can't use .bind() like this for event listeners since we won't be able to remove these event listeners.
::: browser/themes/windows/browser.css
@@ +2841,5 @@
> }
>
> +toolbarpaletteitem {
> + cursor: -moz-grab;
> + transition: -moz-transform, background-color, border-color, box-shadow;
-moz-transform was unprefixed, we can just use `transform`.
Attachment #729296 -
Flags: review?(jAwS) → review-
Assignee | ||
Comment 101•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #77)
> Comment on attachment 729180 [details] [diff] [review]
> Addressing my review comments
>
> Review of attachment 729180 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This looks good to me. Thanks Jared!
>
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +14,5 @@
> > <!-- LOCALIZATION NOTE (mainWindow.titlePrivateBrowsingSuffix): This will be appended to the window's title
> > inside the private browsing mode -->
> > <!ENTITY mainWindow.titlePrivateBrowsingSuffix "(Private Browsing)">
> >
> > +<!ENTITY appmenu.title "Customize and Control &brandFullName;">
>
> I'm assuming this was part of a spec somewhere.
This wasn't part of a spec, but I think that it describes the intentions of the menu and gives better information to the user as to what they can expect before they click on the button.
Reporter | ||
Comment 102•12 years ago
|
||
Comment on attachment 729290 [details] [diff] [review]
After-big-patch Follow-up #3 - Make programmatically generated widgets behave
Review of attachment 729290 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/modules/CustomizeMode.jsm
@@ +221,5 @@
>
> wrapToolbarItem: function(aNode, aPlace) {
> let wrapper = this.createWrapper(aNode, aPlace);
> + // It's possible that this toolbar node is "mid-flight" and doesn't have
> + // a parent, in which case we skip replacing it.
When is this possible? The comment doesn't elaborate.
Attachment #729290 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 103•12 years ago
|
||
Attachment #729598 -
Flags: review+
Reporter | ||
Comment 104•12 years ago
|
||
Comment on attachment 729291 [details] [diff] [review]
After-big-patch Follow-up #4 - Make going into customize mode more awesome
Review of attachment 729291 [details] [diff] [review]:
-----------------------------------------------------------------
Missing OSX/Linux here. As mentioned on IRC, I think we should probably try to have some parity for when this arrives on UX branch - I'd expect there to be a greater percentage of people on OSX using UX-branch than what we expect anywhere else.
Attachment #729291 -
Flags: review?(bmcbride) → review+
Reporter | ||
Comment 105•12 years ago
|
||
Comment on attachment 729291 [details] [diff] [review]
After-big-patch Follow-up #4 - Make going into customize mode more awesome
Review of attachment 729291 [details] [diff] [review]:
-----------------------------------------------------------------
Er, lets make this a r- given that comment.
Attachment #729291 -
Flags: review+ → review-
Reporter | ||
Comment 106•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #71)
> @@ +120,5 @@
> > + this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> > + },
> > +
> > + _defineBuiltInWidgets: function() {
> > + //XXXunf Need a better place to define these.
>
> Is this a better-enough place?
Feels awkward to me. At the very least, it'd be worth moving them to the top of the file, out of the code that implements everything.
Reporter | ||
Comment 107•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #72)
> @@ +344,5 @@
> > + //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> > + // also want to (try) persisting with currentset as well to make it
> > + // less painful to switch to older builds.
> > + persistCurrentSets: function() {
> > + //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)
>
> jaws is right - we should try to fix the binding to inherit from toolkit
> toolbar.
I'm not convinced its worth it (over just re-implementing a simple currentSet) - the toolkit binding has a lot of cruft we don't want.
Reporter | ||
Comment 108•12 years ago
|
||
Comment on attachment 728439 [details] [diff] [review]
Milestone 1 patch
Review of attachment 728439 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/customize.inc
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> + - License, v. 2.0. If a copy of the MPL was not distributed with this
> + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<vbox id="customization-container" flex="1" style="background: #DBEAF9;">
Lets continue to ignore this transgression for now.
::: browser/base/content/panelUI.inc
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> + - License, v. 2.0. If a copy of the MPL was not distributed with this
> + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!-- XXXjaws Need to remove the noautohide="true" -->
Should probably do this on UX-branch.
::: browser/base/content/panelUI.js
@@ +4,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> + "resource:///modules/CustomizableUI.jsm");
> +
> +const PanelUI = {
Could do with some documentation describing what this object is generally for.
@@ +82,5 @@
> + this.panel.removeEventListener(event, this);
> + }
> +
> + this.clickCapturer.removeEventListener("click",
> + this._onCapturerClick.bind(this),
Danger, Will Robinson.
@@ +210,5 @@
> + */
> + _updatePanelButton: function() {
> + if (this.panel.anchorNode) {
> + this.panel.anchorNode.open = this.panel.state == "open" ||
> + this.panel.state == "showing";
Just built Jamun branch, and this doesn't seem to be working for me. ie, the button doesn't become depressed.
::: browser/base/content/panelUI.xml
@@ +1,4 @@
> +<?xml version="1.0"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> + - License, v. 2.0. If a copy of the MPL was not distributed with this
> + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
This file should eventually be merged with the new toolbar.xml (since they're all related to the customizable UI, and fewer documents is less memory consumed).
Attachment #728439 -
Flags: review?(bmcbride) → review+
Updated•12 years ago
|
Attachment #728439 -
Attachment description: Patch (rollup to 77c33bac1b29) → Milestone 1 patch
Comment 109•12 years ago
|
||
Here are all of the fixes for the Milestone 1 patch.
Attachment #729180 -
Attachment is obsolete: true
Attachment #729253 -
Attachment is obsolete: true
Attachment #729598 -
Attachment is obsolete: true
Attachment #729704 -
Flags: review+
Comment 110•12 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #70)
> Comment on attachment 728439 [details] [diff] [review]
> Patch (rollup to 77c33bac1b29)
> ::: browser/themes/linux/panelUIOverlay.css
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +%filter substitution
> > +
> > +%define menuPanelWidth 23em
>
> Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a
> blank line between the %define and the %filter, but in
> windows/panelUIOverlay.css the %define is immediately below the %filter. We
> should make all these files the same.
>
> Also, they all have the same menuPanelWidth. Should that get moved to the
> shared file?
>
Good question - added to my list of things to address in milestone 2.
> ::: browser/themes/shared/panelUIOverlay.inc.css
> @@ +77,5 @@
> > +}
> > +
> > +#PanelUI-subViews {
> > + -moz-stack-sizing: ignore;
> > + background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
>
> This and other places should use the unprefixed syntax. Note that the we
> can't just drop the -moz- to fix this.
Good call - I've deferred this, and added to my list of things to have fixed
during milestone 2.
> @@ +206,5 @@
> > +#customization-palette .toolbarbutton-text {
> > + display: none;
> > +}
> > +
> > +/* XXXunf Hack! Copied from general toolbar styles in winstripe. */
>
> This styling will be used for all platforms. Should the windows theme
> reference this? Or should they just be duplicated since they serve different
> purposes and they may intentionally diverge over time? I'm fine with the
> duplication.
>
Let's leave this for now. OS-specific theming will clear this up.
> We also need the appmenu.png images for linux and osx.
Added to my list of things to get for milestone 2 (or later).
(In reply to Mike Conley (:mconley) from comment #71)
> @@ +120,5 @@
> > + this.registerArea(CustomizableUI.AREA_NAVBAR, ["legacy"]);
> > + },
> > +
> > + _defineBuiltInWidgets: function() {
> > + //XXXunf Need a better place to define these.
>
> Is this a better-enough place?
As per Blair's suggestion, I've (at least for now) put the widget definitions
in a lazy getter at the top of CustomizableUI.jsm.
>
> @@ +253,5 @@
> > + },
> > +
> > + registerArea: function(aName, aProperties) {
> > + if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName))
> > + throw Error("Invalid area name");
>
> throw new Error
>
This used to just return null. Throwing error risks busting the entire UI if
the exception isn't caught. I've opted, at least for now, to log the problem
with ERROR, and continue to return null.
> @@ +328,5 @@
> > + let palette = aAreaNode.toolbox ? aAreaNode.toolbox.palette : null;
> > + let limit = currentNode.previousSibling;
> > + let node = container.lastChild;
> > + while (node != limit) {
> > + // XXXunf Depreciating the old "removable" attribute, is this right?
>
> Not sure. The Social API button, for example, isn't supposed to be
> move-able. I'm not even sure how that is supposed to play in this new
> Australis customization world we're building here. Worth investigating.
>
Added to my list of things to investigate during milestone 2
> @@ +355,5 @@
> > + if (this.isSpecialWidget(aWidgetId))
> > + return CustomizableUI.PROVIDER_SPECIAL;
> > + if (gPalette.has(aWidgetId))
> > + return CustomizableUI.PROVIDER_API;
> > + //XXXunf Er, this isn't entirely true.. could also not exist.
>
> We should handle that case, and log or throw as necessary.
>
Blair explained this more thoroughly, and I've used his comment as the basis
for clearer documentation.
> @@ +552,5 @@
> > + }
> > + return null;
> > + },
> > +
> > + //XXXunf Need to free this from aMenu's tyranny.
>
> Not sure what this comment means, though I do appreciate the use of the word
> tyranny. :) This should either be made more clear, or just be removed.
>
This was replaced with clearer documentation - basically, highlighting the fact
that aMenu shouldn't be necessary.
> @@ +222,5 @@
> > +
> > + this._stowedPalette = this.window.gNavToolbox.palette;
> > + this.window.gNavToolbox.palette = this.visiblePalette;
> > + },
> > + //XXXunf Maybe this should use -moz-element instead of wrapping the node?
>
> I'm not opposed to trying this - makes more sense to me to "imitate"/"mimic"
> the toolbaritems rather than wrapping them. Also means we don't have to
> slurp the items in and out of the invisible palette each time.
>
> Might be worth investigating later.
>
Added to my list of deferred decisions / things to investigate.
> @@ +259,5 @@
> > + }
> > +
> > + this._stowedPalette.appendChild(widgetNode);
> > + } else if (provider = CustomizableUI.PROVIDER_API) {
> > + //XXXunf Currently this doesn't destroy the (now unused) node. It would
>
> Let's just leave this for now, and deal with it in a later iteration.
>
Added to my list of deferred decisions / things to investigate.
> @@ +344,5 @@
> > + //XXXmconley While CustomizableUI.jsm uses prefs to preserve state, we might
> > + // also want to (try) persisting with currentset as well to make it
> > + // less painful to switch to older builds.
> > + persistCurrentSets: function() {
> > + //XXXjaws The toolbar bindings that are included in this changeset (/browser/base/content/toolbar.xml)
>
> jaws is right - we should try to fix the binding to inherit from toolkit
> toolbar.
Instead of inheriting from toolkit's toolbar, we'll implement a simple version
with minimal currentset support.
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #108)
> Comment on attachment 728439 [details] [diff] [review]
> Patch (rollup to 77c33bac1b29)
>
> Review of attachment 728439 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/base/content/customize.inc
> @@ +1,5 @@
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > + - License, v. 2.0. If a copy of the MPL was not distributed with this
> > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +
> > +<vbox id="customization-container" flex="1" style="background: #DBEAF9;">
>
> Lets continue to ignore this transgression for now.
>
Added to my list of deferred things to fix.
> @@ +210,5 @@
> > + */
> > + _updatePanelButton: function() {
> > + if (this.panel.anchorNode) {
> > + this.panel.anchorNode.open = this.panel.state == "open" ||
> > + this.panel.state == "showing";
>
> Just built Jamun branch, and this doesn't seem to be working for me. ie, the
> button doesn't become depressed.
>
Good call. this.panel.anchorNode has no notion of open / close. Fixed this.
> ::: browser/base/content/panelUI.xml
> @@ +1,4 @@
> > +<?xml version="1.0"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > + - License, v. 2.0. If a copy of the MPL was not distributed with this
> > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>
> This file should eventually be merged with the new toolbar.xml (since
> they're all related to the customizable UI, and fewer documents is less
> memory consumed).
Noted in my list of things to fix later.
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #94)
> Comment on attachment 729253 [details] [diff] [review]
> Addressing my review comments (apply on top of jaws' patch)
>
> Review of attachment 729253 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> @@ +110,5 @@
> > + this._onDragStart.bind(this));
> > + this.visiblePalette.removeEventListener("dragover",
> > + this._onDragOver.bind(this));
> > + this.visiblePalette.removeEventListener("drop",
> > + this._onDragDrop.bind(this));
>
> Fun fact: this.someFunction.bind(this) !== this.someFunction.bind(this)
>
> Therefore, removing event listeners like this doesn't work, because you're
> trying to remove a different listener :( Need to store the value you
> originally pass into addEventListener() if you want to use bind().
>
> Same issue later in this function too.
Gah, embarrassing to only find this out now. :/ I've fixed this, and will fix
it in the follow-up patches as well.
Comment 111•12 years ago
|
||
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #102)
> Comment on attachment 729290 [details] [diff] [review]
> After-big-patch Follow-up #3 - Make programmatically generated widgets behave
>
> Review of attachment 729290 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/modules/CustomizeMode.jsm
> @@ +221,5 @@
> >
> > wrapToolbarItem: function(aNode, aPlace) {
> > let wrapper = this.createWrapper(aNode, aPlace);
> > + // It's possible that this toolbar node is "mid-flight" and doesn't have
> > + // a parent, in which case we skip replacing it.
>
> When is this possible? The comment doesn't elaborate.
I've added a clearer comment:
"It's possible that this toolbar node is "mid-flight" and doesn't have a parent, in which case we skip replacing it. This can happen if a toolbar item has been dragged into the palette. In that case, we tell CustomizableUI to remove the widget from its area before putting the widget in the palette - so the node will have no parent."
Comment 112•12 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #97)
> Comment on attachment 729292 [details] [diff] [review]
> After-big-patch Follow-up #5 - More ways to exit customize mode
>
> Review of attachment 729292 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: browser/modules/CustomizeMode.jsm
> @@ +361,5 @@
> > + this.exit();
> > + }
> > + break;
> > + case "click":
> > + if (aEvent.originalTarget = this.window.PanelUI.menuButton) {
>
> This should also check aEvent.button == 0.
Done. Also, just noticed I used the assignment operator instead of the equivalence operator. Fixed that too.
Comment 113•12 years ago
|
||
Attachment #729287 -
Attachment is obsolete: true
Comment 114•12 years ago
|
||
Comment 115•12 years ago
|
||
Attachment #729289 -
Attachment is obsolete: true
Attachment #729290 -
Attachment is obsolete: true
Comment 116•12 years ago
|
||
Attachment #729291 -
Attachment is obsolete: true
Comment 117•12 years ago
|
||
Comment on attachment 729735 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome
Copied the CSS from windows/browser.css over to osx and linux. Added a note to revisit OS specific theme-ing in a future milestone (falls under "Make pretty").
Attachment #729735 -
Flags: review?(jAwS)
Comment 118•12 years ago
|
||
Attachment #729292 -
Attachment is obsolete: true
Comment 119•12 years ago
|
||
Attachment #729737 -
Flags: review?(jAwS)
Comment 120•12 years ago
|
||
Attachment #729293 -
Attachment is obsolete: true
Attachment #729294 -
Attachment is obsolete: true
Comment 121•12 years ago
|
||
Attachment #729295 -
Attachment is obsolete: true
Comment 122•12 years ago
|
||
Attachment #729296 -
Attachment is obsolete: true
Attachment #729742 -
Flags: review?(jAwS)
Assignee | ||
Comment 123•12 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #110)
> (In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #70)
> > Comment on attachment 728439 [details] [diff] [review]
> > Patch (rollup to 77c33bac1b29)
> > ::: browser/themes/linux/panelUIOverlay.css
> > @@ +3,5 @@
> > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +%filter substitution
> > > +
> > > +%define menuPanelWidth 23em
> >
> > Uber nit, the linux/panelUIOverlay.css and osx/panelUIOverlay.css have a
> > blank line between the %define and the %filter, but in
> > windows/panelUIOverlay.css the %define is immediately below the %filter. We
> > should make all these files the same.
> >
> > Also, they all have the same menuPanelWidth. Should that get moved to the
> > shared file?
> >
>
> Good question - added to my list of things to address in milestone 2.
I fixed the uber nit already in "addressed review comments" patch but didn't adjust the location of menuPanelWidth.
> > ::: browser/themes/shared/panelUIOverlay.inc.css
> > @@ +77,5 @@
> > > +}
> > > +
> > > +#PanelUI-subViews {
> > > + -moz-stack-sizing: ignore;
> > > + background-image: -moz-linear-gradient(center top , white 1px, rgba(255, 255, 255, 0) 15px); /* stolen from globa/skin/popup.css */
> >
> > This and other places should use the unprefixed syntax. Note that the we
> > can't just drop the -moz- to fix this.
>
> Good call - I've deferred this, and added to my list of things to have fixed
> during milestone 2.
Already fixed now.
>
> > We also need the appmenu.png images for linux and osx.
>
> Added to my list of things to get for milestone 2 (or later).
Already fixed.
Assignee | ||
Comment 124•12 years ago
|
||
Comment on attachment 729735 [details] [diff] [review]
M1 - Follow-up #4 - Make going into customize mode more awesome
Review of attachment 729735 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/themes/windows/jar.mn
@@ +24,5 @@
> skin/classic/browser/appmenu-icons.png
> skin/classic/browser/appmenu-dropmarker.png
> * skin/classic/browser/browser.css
> skin/classic/browser/click-to-play-warning-stripes.png
> + skin/classic/browser/customization/customization-mode-background.jpg (../shared/customization/customization-mode-background.jpg)
As discussed on IRC, we should just have separate copies of this graphic for the three platforms. This should be more consistent with the rest of the file.
Attachment #729735 -
Flags: review?(jAwS) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #729737 -
Flags: review?(jAwS) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #729742 -
Flags: review?(jAwS) → review+
Comment 125•12 years ago
|
||
Moved the customization grid background texture to the individual theme folders and updated the jar.mn files.
Attachment #729735 -
Attachment is obsolete: true
Comment 126•12 years ago
|
||
I had to fix these to make the panel look more acceptable on OSX and Linux.
Updated•12 years ago
|
Attachment #729874 -
Flags: review?(jAwS)
Assignee | ||
Comment 127•12 years ago
|
||
Comment on attachment 729874 [details] [diff] [review]
M1 - Follow-up #10 - Make buttons look right on OSX and Linux, and make grid texture work on Linux
Review of attachment 729874 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/themes/linux/panelUIOverlay.css
@@ +33,5 @@
> + padding-left: 12px;
> + padding-right: 12px;
> +}
> +
> +#PanelUI-zoomIn-btn {
zoomIn and zoomOut can get disabled when the user has reached the zoom limit, right?
Attachment #729874 -
Flags: review?(jAwS) → review+
Reporter | ||
Comment 128•12 years ago
|
||
(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #127)
> zoomIn and zoomOut can get disabled when the user has reached the zoom
> limit, right?
No - neither these controls or the original ones become disabled when you reach the max in either direction (in fact, doesn't look like the menuitems in the main menu do either). I think they ideally should get disabled, but that's an existing bug - not something introduced here.
Comment 129•12 years ago
|
||
The patches in this bug landed on the jamun branch as follows (in order from Milestone 1 patch, to follow-up #10):
https://hg.mozilla.org/projects/jamun/rev/b69ac9d75fc7
https://hg.mozilla.org/projects/jamun/rev/4ed082add187
https://hg.mozilla.org/projects/jamun/rev/2beaee0d7ea0
https://hg.mozilla.org/projects/jamun/rev/b3ff9b4914e6
https://hg.mozilla.org/projects/jamun/rev/dd9f1c37b78e
https://hg.mozilla.org/projects/jamun/rev/e3f092d97b12
https://hg.mozilla.org/projects/jamun/rev/761af0165f16
https://hg.mozilla.org/projects/jamun/rev/76a9a142656b
https://hg.mozilla.org/projects/jamun/rev/60ff4fbb2db1
https://hg.mozilla.org/projects/jamun/rev/db353936084c
and then onto the UX branch as:
https://hg.mozilla.org/projects/ux/rev/b69ac9d75fc7
https://hg.mozilla.org/projects/ux/rev/4ed082add187
https://hg.mozilla.org/projects/ux/rev/2beaee0d7ea0
https://hg.mozilla.org/projects/ux/rev/b3ff9b4914e6
https://hg.mozilla.org/projects/ux/rev/dd9f1c37b78e
https://hg.mozilla.org/projects/ux/rev/e3f092d97b12
https://hg.mozilla.org/projects/ux/rev/761af0165f16
https://hg.mozilla.org/projects/ux/rev/76a9a142656b
https://hg.mozilla.org/projects/ux/rev/60ff4fbb2db1
https://hg.mozilla.org/projects/ux/rev/db353936084c
https://hg.mozilla.org/projects/ux/rev/43a95bc2eddb
Comment 130•12 years ago
|
||
Missed one - this is follow-up #10 on jamun:
https://hg.mozilla.org/projects/jamun/rev/43a95bc2eddb
Updated•12 years ago
|
Updated•12 years ago
|
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [fixed-in-ux]
Updated•12 years ago
|
Blocks: australis-cust
Comment 132•12 years ago
|
||
Is the menu button movable, rather than being forced in a right alligned.. Really for ex-Australis users since I prefer having my menu options on the left, and no doubt other users would want something similar.. Is there a Bugzilla ID for this?
Assignee | ||
Comment 133•12 years ago
|
||
(In reply to mdew from comment #132)
> Is the menu button movable, rather than being forced in a right alligned..
> Really for ex-Australis users since I prefer having my menu options on the
> left, and no doubt other users would want something similar.. Is there a
> Bugzilla ID for this?
I don't know of a bug being filed for it, but there are no plans for this functionality to be included in Firefox by default.
Comment 134•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #133)
>
> I don't know of a bug being filed for it, but there are no plans for this
> functionality to be included in Firefox by default.
Given all incarnations of Firefox have been left-aligned up till now, I dont mind having right-aligned being the default.I just prefer having my menus (menu option/bookmark button) on the left. Such a radical change, non configurable change, may annoy existing firefox users.
Comment 135•12 years ago
|
||
I think we definitely want it non-removable by default (like certain other important navbar items), but I'm sure sure there's any reason to disallow moving it. (OTOH, it doesn't seem like a big deal, since the Firefox button wasn't movable.)
Related: where's it get shown in RTL mode? :)
Comment 136•12 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #135)
> Related: where's it get shown in RTL mode? :)
It's correctly on the left-hand side. The subview transitions are wrong though (bug 872548).
Comment 137•12 years ago
|
||
> OTOH, it doesn't seem like a big deal, since the Firefox button wasn't movable.
The Firefox button was on the tab bar, though. One of the reasons why some people may want to move the menu button is that they use extensions to hide the nav bar for pinned tabs - a feature which was actually planned to be included by default, at one point.
Comment 138•12 years ago
|
||
I'm doing that myself. It will be inconvenient, yes, but I can live with it.
Comment 139•12 years ago
|
||
I was referring to Windows, where the bright orange Firefox button has always been at the top-left and non-customizable/movable.
Comment 140•12 years ago
|
||
What I meant was that the Firefox button, though not customizable, was guaranteed to be always visible. This isn't the case with the new menu button because the navigation bar can be hidden and in fact some users do hide it regularly for pinned tabs. These users might want to move the menu button to the tab bar.
Assignee | ||
Comment 141•12 years ago
|
||
(In reply to Adam Kowalczyk from comment #140)
> What I meant was that the Firefox button, though not customizable, was
> guaranteed to be always visible. This isn't the case with the new menu
> button because the navigation bar can be hidden and in fact some users do
> hide it regularly for pinned tabs. These users might want to move the menu
> button to the tab bar.
The navigation bar cannot be hidden anymore via the menus or context menu (bug 870545) and we no longer hide the navigation bar when viewing the add-ons manager (bug 752434).
Updated•12 years ago
|
Keywords: dev-doc-needed
Updated•11 years ago
|
Summary: New PanelUI and toolbar customization → New PanelUI and toolbar customization (Milestone 1)
Comment 142•11 years ago
|
||
Security Review finished, see bug 853453.
Flags: sec-review?(fbraun) → sec-review+
Comment 143•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7cb839f3bfc
https://hg.mozilla.org/mozilla-central/rev/ba782083b7a5
https://hg.mozilla.org/mozilla-central/rev/ce10b31bac0f
https://hg.mozilla.org/mozilla-central/rev/9ba965255976
https://hg.mozilla.org/mozilla-central/rev/57ed4ef40b14
https://hg.mozilla.org/mozilla-central/rev/5d77bc6e8769
https://hg.mozilla.org/mozilla-central/rev/d251a7608f71
https://hg.mozilla.org/mozilla-central/rev/18d701f03aa9
https://hg.mozilla.org/mozilla-central/rev/81c6819102e9
https://hg.mozilla.org/mozilla-central/rev/ac6747aa84dd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Restrict Comments: false
Whiteboard: [fixed-in-ux]
Target Milestone: --- → Firefox 28
You need to log in
before you can comment on or make changes to this bug.
Description
•