It'd be nice to get the In Content Prefs styling for the add-on manager for consistency and a more modern look.

Michael, can you provide some assets/designs for this bug ? Thanks :)

Tim, this work is in the planning stage. Will keep you updated :)

Blair, do you think this bug is specific to Firefox or common to all Toolkit apps? In the latter case it would need moving to Toolkit::Add-ons Manager.

Yea, Toolkit. Firefox won't get getting it's own special theme for the Add-ons Manager. Plan is to start with Firefox to flesh out the design and implement a newer common CSS framework in /browser/themes/*/in-content/. Once that's working reasonably smoothly, we can start work on moving it over to Toolkit and adapting the Add-ons Manager - which is *by far* the most complex in-content page in the tree. So, still a decent amount of work to do before this bug is in an actionable state.

(In reply to Jared Wein [:jaws] (please needinfo? me) from bug 1013346 comment #0) > Created attachment 8425558 [details] > Backup of Chameleon style guide > > For consistency, the add-ons manager should be updated to match the same > in-content styling of the new in-content preferences. The style guide is > available at http://people.mozilla.org/~jgruen/chameleon/#nav0

With project chameleon released, is this now ready for work to proceed?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8) > With project chameleon released, is this now ready for work to proceed? It still needs icons and the searchbox placement to study.

Jared, we should move this bug into the priority backlog.

Yes, as it is blocking shipping in-content prefs and is a somewhat larger task than the small bugfixes in the in-content prefs.

Tim, do you have already worked on this bug? Or do you mind when I take this bug? I have first patches ready which are awaiting the landing of bug 1000625. The screenshot shows the actual state.

(In reply to Richard Marti (:Paenglab) from comment #12) > Created attachment 8469150 [details] > newAddOnManager.png > > Tim, do you have already worked on this bug? Or do you mind when I take this > bug? I have first patches ready which are awaiting the landing of bug > 1000625. > > The screenshot shows the actual state. I was waiting for bug 1000625. But since you have patches ready, you can take the bug.

This is only the move of in-content/common.css from browser to global. Blair, after review, could this ASAP? Then third party themers don't have to support common.css in browser and global for the in-content prefs.

This is my first shot of Add-on manager restyle. Is this what you have thought? I've seen no mockup, so I styled it what I think it could look.

Comment on attachment 8469150 [details] newAddOnManager.png I think you should use less margin, especially on the top of the content. Power users usually have lots of addons, and this is rather a regression for them.

Comment on attachment 8469946 [details] [diff] [review] 989469-useNewCommonStyle.patch Review of attachment 8469946 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/extensions/extensions.inc.css @@ +56,5 @@ > } > > #detail-view .global-warning { > padding: 4px 12px; > + border-bottom: 1px solid #c1c1c1; I'll remove this whitespace in the next patch.

(In reply to Tim Nguyen [:ntim] from comment #16) > Comment on attachment 8469150 [details] > newAddOnManager.png > > I think you should use less margin, especially on the top of the content. > Power users usually have lots of addons, and this is rather a regression for > them. On the sides it's the same margin as the prefs are using and I think this should be stay. The header has now 30px on top and bottom. Maybe 20px would also work but less could make the header looking crammed. I'll search today, then they are up, someone from UX which could do a first ui-r.

I made a try build: https://tbpl.mozilla.org/?tree=Try&rev=05bceb06a9ef and saw on OS X a wrong padding of menulists on Plugin page -> fixed locally. bc1 failed but I think this has nothing to do with my patches. bc3 also failed but I don't know why this fails. Please can someone with tests experience look in it what is wrong?

Fixed comment 17 and 19

Comment on attachment 8470040 [details] [diff] [review] 989469-useNewCommonStyle.patch This ui-r? is meant only for a first check with feedback what should be changed. The category icons aren't the correct and will be updated from bug 1048912.

I think you could also ask Mike Maslaney since he is in charge of project Chameleon.

Updated patch after Stephen's proposal to merge the add-ons together and use only a line as divider. Stephen, could you also check what background color should be used for the plugin check header on Plugin page and for the contribution container on some Add-ons details page?

Fixed the path to sorter.png like in bug 1052315.

Comment on attachment 8471775 [details] [diff] [review] 989469-moveCommonToGlobal.patch - checked-in (comment 28) Review of attachment 8471775 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in getting to this!

Try: https://tbpl.mozilla.org/?tree=Try&rev=6526c11ca4ab

You can just set the checkin flag to + to indicate that it's been checked in.

Updated to tip.

Comment on attachment 8474175 [details] Screenshot - blocklisted plugin (Bah, attachment upload fail) For a blocklisted item, the "More Information" link's font size is too big.

The information bars at the top should have something extra to make them more defined. With the border missing, it just blends in too much to the page background. Also, these info bars originally did not screen with the contents of the view. I think we should keep that behaviour.

Another info bar as an example.

Can now end up with two scrolling areas in the details view - one inside another. I think this is likely due to changing the info bars to be part of the area that scrolls (we used to have the same bug ages ago) - so I think reverting that behaviour should fix this.

In the search view, the line between the "My Add-ons"/"Available add-ons" selector and the list of search results is now double thickness.

In the details view, we set a background that fades away with the add-on is in a pending state, a warning state, or disabled. This now looks a bit odd due to having no border to define that area, especially if the background is one of the striped ones (pending or warning states). At the very least, the background needs to extend to to the left edge so it touches the right edge of the categories. Also, with the lack of a border between the view area and the top header, I think the amount of whitespace needs to be reduced (between the top header and the add-on name).

When there is only one item in the listview, it looks a bit broken - it's not clear that this is in fact a list with one item. Maybe a different background color would help this.

Comment on attachment 8474080 [details] [diff] [review] 989469-useNewCommonStyle.patch Review of attachment 8474080 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: toolkit/mozapps/extensions/content/extensions.xul @@ +123,5 @@ > <hbox flex="1"> > > <!-- category list --> > <richlistbox id="categories"> > + <hbox id="nav-header" align="center" pack="center"> This shouldn't be a child of the richlistbox. Also, these back/forward buttons look really out of place now - need to restyle them so they fit in to this area. These are used in applications that open about:addons in it's own window, rather than a tab - can test this mode by running the following: window.openDialog("about:addons");

(In reply to Blair McBride [:Unfocused] from comment #32) > Comment on attachment 8474175 [details] > Screenshot - blocklisted plugin > > (Bah, attachment upload fail) > > For a blocklisted item, the "More Information" link's font size is too big. Fixed (In reply to Blair McBride [:Unfocused] from comment #33) > Created attachment 8474177 [details] > Screenshot - plugin check > > The information bars at the top should have something extra to make them > more defined. With the border missing, it just blends in too much to the > page background. I asked Stephen in comment 23 for a color and how this should look. > Also, these info bars originally did not screen with the contents of the > view. I think we should keep that behaviour. I don't understand what you mean. (In reply to Blair McBride [:Unfocused] from comment #35) > Created attachment 8474179 [details] > Screenshot - detail view scrolling > > Can now end up with two scrolling areas in the details view - one inside > another. I think this is likely due to changing the info bars to be part of > the area that scrolls (we used to have the same bug ages ago) - so I think > reverting that behaviour should fix this. This should be fixed now. (In reply to Blair McBride [:Unfocused] from comment #36) > Created attachment 8474180 [details] > Screenshot - search > > In the search view, the line between the "My Add-ons"/"Available add-ons" > selector and the list of search results is now double thickness. Hmm, here it is 1px. (In reply to Blair McBride [:Unfocused] from comment #38) > Created attachment 8474188 [details] > Screenshot - list with one item > > When there is only one item in the listview, it looks a bit broken - it's > not clear that this is in fact a list with one item. Maybe a different > background color would help this. I gave now the listview a lighter background color to show the listview area. Stephen, is this okay or what do you propose should this be done? (In reply to Blair McBride [:Unfocused] from comment #39) > Comment on attachment 8474080 [details] [diff] [review] > 989469-useNewCommonStyle.patch > > Review of attachment 8474080 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice! > > ::: toolkit/mozapps/extensions/content/extensions.xul > @@ +123,5 @@ > > <hbox flex="1"> > > > > <!-- category list --> > > <richlistbox id="categories"> > > + <hbox id="nav-header" align="center" pack="center"> > > This shouldn't be a child of the richlistbox. I was afraid the categories don't shrink correctly on small browser width, but I found a solution now. > Also, these back/forward buttons look really out of place now - need to > restyle them so they fit in to this area. These are used in applications > that open about:addons in it's own window, rather than a tab - can test this > mode by running the following: I copied now the white arrows from toolbar-inverted.png and on hover they have background-color: #ebebeb as feedback. > window.openDialog("about:addons"); Or use Thunderbird :)

(In reply to Richard Marti (:Paenglab) from comment #40) > > Also, these info bars originally did not screen with the contents of the > > view. I think we should keep that behaviour. > > I don't understand what you mean. Er, scroll. The info bars did not *scroll*.

They don't scroll. Maybe they scrolled because of the double scrollbars.

Comment on attachment 8474256 [details] [diff] [review] 989469-useNewCommonStyle.patch Think I need to hear from Stephen before I can do much more here.

Patch updated to tip.

Comment on attachment 8480672 [details] [diff] [review] 989469-useNewCommonStyle.patch Bouncing this over to Philipp to try get some traction here.

Comment on attachment 8474188 [details] Screenshot - list with one item This image (and the other images) look good to me! I can't apply the patches though, so I can't test it live. Do you happen to have a build (preferably Mac) for this? One thing we'll definitely want to do is to create new icons that fit the style, but that can be done in a follow-up. Michael, could you also have a look here and see if you spot any red flags?

Huh, for some reason applying now worked :) There are some issues which I've described in the attachment. Not sure what to do about the issue that deactivated items are emphasized instead of de-emphasized. Michael likely has directions here.

(In reply to Philipp Sackl [:phlsa] from comment #46) > Comment on attachment 8474188 [details] > Screenshot - list with one item > > This image (and the other images) look good to me! > I can't apply the patches though, so I can't test it live. Do you happen to > have a build (preferably Mac) for this? > > One thing we'll definitely want to do is to create new icons that fit the > style, but that can be done in a follow-up. > > Michael, could you also have a look here and see if you spot any red flags? I think the list with one item still looks a bit weird here, it doesn't really look like an actual list item.

Comment on attachment 8474188 [details] Screenshot - list with one item Clearing review on this for now, since it will probably change with the things I mentioned in my last comment.

(In reply to Philipp Sackl [:phlsa] from comment #46) > > One thing we'll definitely want to do is to create new icons that fit the > style, but that can be done in a follow-up. This would be bug 1048912. (In reply to Philipp Sackl [:phlsa] from comment #47) > Created attachment 8489347 [details] > Issues > > There are some issues which I've described in the attachment. > Not sure what to do about the issue that deactivated items are emphasized > instead of de-emphasized. Michael likely has directions here. I changed the colors of the inactive extensions to the background color and the active ones to a lighter color. Both when clicked are a little bit lighter. Is this okay. The header-utils-btn should now also have a complete border. I made a try build for tests with latest changes. I haven't tested on OS X and because of this I don't attach the patch now. https://bugzilla.mozilla.org/show_bug.cgi?id=989469

Patch I mentioned in comment 50. I see I had added the wrong link to the try build. Here the correct: https://tbpl.mozilla.org/?tree=Try&rev=211334373f7f This patch adds back the -moz-user-select: none; on some elements (which isn't in try build). I hoped bug 1055873 would be faster fixed, but I can remove them later.

Michael, for your convenience, here's a direct link to the Mac build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/richard.marti@gmail.com-211334373f7f/try-macosx64/firefox-35.0a1.en-US.mac.dmg

Unbitrot

Unbitrotted patch

Comment on attachment 8513300 [details] [diff] [review] 989469-useNewCommonStyle.patch Feedback http://people.mozilla.org/~mmaslaney/incontent/Incontent-addons-themes.png http://people.mozilla.org/~mmaslaney/incontent/Incontent-addons-profile.png http://people.mozilla.org/~mmaslaney/incontent/Incontent-addons-search.png I'm not sure when Fira Sans will land, but you can hold off on those changes until further notice.

Feedback addressed. I've added a box-shadow around the details image like in your middle screenshot. Is that okay? I also started a try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47c3ff73f586

I'm fine with removing the light gray background, but I'm afraid we won't see which element is focused. Maybe add a blue border on the left of the focused item ?

The focused element has a lighter background.

Some interesting elements that could fix some problems raised here : https://people.mozilla.org/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-InContentUI-%28Addons-Manager%29.jpg

ui-r bump Michael, please could you look at this? Now with the plan to enable the in-content prefs it would be good to have a consistent look between them an the add-on manager. I think a issue is now there is almost no difference between selected and unselected item. What do you think about ntim's comment 57?

I meant: with the plan to enable the in-content prefs in FX 38

Unbitrott and made the add-on text difference bigger to differentiate more between selected- and unselected add-on.

This patch is using the same border on the left as the categories to show the selected add-on.

Screenshot with the border on the left for the selected add-on.

Richard, while you're working on this, can you also take care of bug 1022600 ?

Comment on attachment 8553297 [details] [diff] [review] 989469-useNewCommonStyle.patch Can you provide a screenshot?

Comment on attachment 8553299 [details] [diff] [review] 989469-useNewCommonStyleWithBorder.patch Can you provide a screenshot?

(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #67) > Comment on attachment 8553297 [details] [diff] [review] > 989469-useNewCommonStyle.patch > > Can you provide a screenshot? This is the screenshot with the different text color (it is from my Thunderbird but it is the same as from FX).

Comment on attachment 8553300 [details] selected-border.png (In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #68) > Comment on attachment 8553299 [details] [diff] [review] > 989469-useNewCommonStyleWithBorder.patch > > Can you provide a screenshot? This screenshot shows the orange border to highlight the selected add-on.

Comment on attachment 8554720 [details] selected-color.png Unless there's a design decision for placing the buttons at the button — I'm assuming we need the space for long headers — could we have them vertically centered and make a column break for the text?

Comment on attachment 8554720 [details] selected-color.png Unless there's a design decision for placing the buttons at the button — I'm assuming we need the space for long headers — could we have them vertically centered and make a column break for the text?

Comment on attachment 8553300 [details] selected-border.png Unless there's a design decision for placing the buttons at the button — I'm assuming we need the space for long headers — could we have them vertically centered and make a column break for the text?

Michael, which one should I use for the selected add-on? The one with the different text color or the one with the orange border?

I prefer the orange border. Nice touch.

Comment on attachment 8553299 [details] [diff] [review] 989469-useNewCommonStyleWithBorder.patch Trying to vertically center the buttons and asking then for review.

Is this what you thought with the centered buttons? I've chosen the plugins with the long names to show how it looks when the name is to long (the full name is also in the tooltip).

I prefer the center alignment, thanks for updating. I would add a little more padding between the title and the right most button.

I think, with the positive feedback on ui, it's time to ask for review.

[Tracking Requested - why for this release]: We'll enable in-content prefs in 38, so it makes sense to track this work for 38 as well

Comment on attachment 8558683 [details] [diff] [review] 989469-useNewCommonStyle.patch Dave: Do you have any free cycles to look at this? I'm swamped with the ReadingList project. Mostly interested in having someone to go through looking at all the edge cases in the UI, that other people wouldn't think to look for.

Updated to tip.

Comment on attachment 8562342 [details] [diff] [review] 989469-useNewCommonStyle.patch Review of attachment 8562342 [details] [diff] [review]: ----------------------------------------------------------------- This is a long patch so it might take me longer to go through than I'd like. At first glance it looks like there are a lot of whitespace only changes here, it might make things a lot easier if you could attach a whitespace only patch for me to look at (hg diff -w).

I think we need to think more about distinguishing the list from the rest of the page. When you have just one theme installed the page looks very empty just with the one theme off in a strange place. Going to the default theme's details looks bad in similar ways. It's bad in the current theme too but these changes make it look worse because there is less content around it.

There seem to be a couple of bugs with focus rings showing up on OSX when you click on the list: https://www.dropbox.com/s/nebog6cs4j0vikp/Screenshot%202015-02-12%2013.55.55.png?dl=0 https://www.dropbox.com/s/sutvl1akt4lhjrd/Screenshot%202015-02-12%2013.58.28.png?dl=0

diff -w patch

My normal patch had some lines removed I didn't want. This one has this corrected and is the same as the diff -w one (attachment 8563669 [details] [diff] [review]).

Comment on attachment 8563656 [details] Screenshot 2015-02-12 13.55.48.png I agree, it does look a bit lonely out there by itself. Adding a light gray bottom-rule would definitely ground the element a bit more to the page.

Bottom border when only one extension added.

The same but normal patch.

Screenshot with only one extension. The border color is the same as between multiple extensions (#c1c1c1).

Another oddity, in the get add-ons view there is a big gap at the bottom. With no border around the content it's a little weird seeing the content cut off at the top too: https://www.dropbox.com/s/93njz7gho9704fr/Screenshot%202015-03-06%2016.24.33.png?dl=0

Not having any kind of background change on selecting an item in the list, just the coloured bar at the side feels weird to me. Is there a reason we aren't doing more there?

The column headers in the search and recent updates view look squashed over to the right, the search icon looks like there is something unintentional behind it: https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04.png?dl=0

Comment on attachment 8564290 [details] [diff] [review] 989469-useNewCommonStyle.patch Review of attachment 8564290 [details] [diff] [review]: ----------------------------------------------------------------- I'm not seeing anything obviously wrong with the patch aside from the problems I noted in comment 85, comment 92 and comment 94. I'd like to see this patch again with those fixed but it will probably be a quick review now I've been over it once. I'd also like to hear some answer to comment 93. Chances are we're going to find more things that are wrong with this by landing it and getting it into the hands of nightly testers.

(In reply to Dave Townsend [:mossop] from comment #85) > There seem to be a couple of bugs with focus rings showing up on OSX when > you click on the list: > > https://www.dropbox.com/s/nebog6cs4j0vikp/Screenshot%202015-02-12%2013.55.55. > png?dl=0 > https://www.dropbox.com/s/sutvl1akt4lhjrd/Screenshot%202015-02-12%2013.58.28. > png?dl=0 This comes from https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#660. I could remove this border. When I should leave it, then I see the problem on the right where is no margin to the window. When I would add the margin, then the richlist scrollbar would also move and we would again have the complaints the scrollbar isn't at the edge. (In reply to Dave Townsend [:mossop] from comment #93) > Not having any kind of background change on selecting an item in the list, > just the coloured bar at the side feels weird to me. Is there a reason we > aren't doing more there? This was decided in comment 75 (In reply to Dave Townsend [:mossop] from comment #94) > The column headers in the search and recent updates view look squashed over > to the right, the search icon looks like there is something unintentional > behind it: > https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04. > png?dl=0 The headers are on the same position as before. Michael, would it be better to move them to the left above the search radio buttons? The search icon is the same as now and with the light background this "something" isn't so obvious. With bug 1048912 the icons should change and look like the in-content pref icons.

(In reply to Richard Marti (:Paenglab) from comment #96) > (In reply to Dave Townsend [:mossop] from comment #85) > > There seem to be a couple of bugs with focus rings showing up on OSX when > > you click on the list: > > > > https://www.dropbox.com/s/nebog6cs4j0vikp/Screenshot%202015-02-12%2013.55.55. > > png?dl=0 > > https://www.dropbox.com/s/sutvl1akt4lhjrd/Screenshot%202015-02-12%2013.58.28. > > png?dl=0 > > This comes from > https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in- > content/common.inc.css#660. > > I could remove this border. When I should leave it, then I see the problem > on the right where is no margin to the window. When I would add the margin, > then the richlist scrollbar would also move and we would again have the > complaints the scrollbar isn't at the edge. So the second case is a particular problem, we shouldn't be showing the richlist at all there I think. The first case is particularly bad because with no border there to begin with when the list gets focus all the content shifts down and right a pixel. At the least the list should start with a 1 pixel transparent border (that should probably be defined in common.inc.css) which will stop the shifting. Other examples I can find actually have a border which makes the change look a lot less jarring, look at the search providers list in about:preferences for example. > (In reply to Dave Townsend [:mossop] from comment #94) > > The column headers in the search and recent updates view look squashed over > > to the right, the search icon looks like there is something unintentional > > behind it: > > https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04. > > png?dl=0 > > The headers are on the same position as before. > Michael, would it be better to move them to the left above the search radio > buttons? Hmm you're right. I think my confusion is that the old ones look more like buttons, the new ones look more like table headers. Or maybe the old ones were a bad design and I just got used to them ;). Let's see what Mike says. > The search icon is the same as now and with the light background this > "something" isn't so obvious. With bug 1048912 the icons should change and > look like the in-content pref icons. Ok great.

Comment on attachment 8564291 [details] one-extension.png Richard, Dave, how far is this from completion? We are currently doing some design work for changes to the add-on manager of course it would be better to do that with the new awesome stuff in place :) The one-line case looks fine.

(In reply to Philipp Sackl [:phlsa] from comment #98) > Comment on attachment 8564291 [details] > one-extension.png > > Richard, Dave, how far is this from completion? > We are currently doing some design work for changes to the add-on manager of > course it would be better to do that with the new awesome stuff in place :) We could still do with UX input on the sort headers talked about in comments 94, 96 and 97

(In reply to Dave Townsend [:mossop] from comment #94) > The column headers in the search and recent updates view look squashed over > to the right, the search icon looks like there is something unintentional > behind it: > https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04. > png?dl=0 Could you provide me with an updated screen shot? I'm not seeing the squashed headers.

(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #100) > (In reply to Dave Townsend [:mossop] from comment #94) > > The column headers in the search and recent updates view look squashed over > > to the right, the search icon looks like there is something unintentional > > behind it: > > https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04. > > png?dl=0 > > Could you provide me with an updated screen shot? I'm not seeing the > squashed headers. Bugzilla mangles the long link, here is a shortened one: http://bit.ly/1OVF6LW

(In reply to Philipp Sackl [:phlsa] from comment #98) > Comment on attachment 8564291 [details] > one-extension.png > > Richard, Dave, how far is this from completion? > We are currently doing some design work for changes to the add-on manager of > course it would be better to do that with the new awesome stuff in place :) I'm waiting for the header decision for a new patch. The other comments from Dave are addressed locally.

For the search icon, I think we should wait for bug 1148749 to look consistent.

Updated to tip after the override changes. This patch has all changes from Dave's comment's except the header changes (which are awaiting Michael's decision).

The same patch with diff -w created.

Dave, my latest patch fails now on tests because I'm now hiding the lists when the EmptyNotice is shown: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f18d3a07a0c Please could you look at this tests? I'm not so good in JS and tests. If you could fix them, I would be very thankful.

And again updated after bit rot.

diff -w patch

(In reply to Dave Townsend [:mossop] from comment #101) > (In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from > comment #100) > > (In reply to Dave Townsend [:mossop] from comment #94) > > > The column headers in the search and recent updates view look squashed over > > > to the right, the search icon looks like there is something unintentional > > > behind it: > > > https://www.dropbox.com/s/86g69jw4amm2k3g/Screenshot%202015-03-06%2016.36.04. > > > png?dl=0 > > > > Could you provide me with an updated screen shot? I'm not seeing the > > squashed headers. > > Bugzilla mangles the long link, here is a shortened one: > http://bit.ly/1OVF6LW The headers look good to me. +1 for UI.

Comment on attachment 8588011 [details] [diff] [review] 989469-useNewCommonStyle.patch Thank you Michael. Adding r? as the other comments from Dave are fixed. Dave, please could you look at comment 106? I'm not so safe in JS and especially in tests.

I took a brief look yesterday and it based on the logs I'd guess the list isn't getting re-displayed when a new install is added while the manager is already open but I didn't get much further than that. I'm swamped with some high priority work but I'm hoping to look at this later this week.

I won't track this bug anymore. We are now in the beta cycle and it is too late for new feature.

Actually we end up adding the same install to the list multiple times. Seems like when the listbox is unhidden its XBL isn't getting set up quickly enough. This switches to using DOM APIs for clearing the existing contents. It fixes the first failing test so that might fix later ones too, see how it does.

Patch with XBL fixup included.

Comment on attachment 8589883 [details] [diff] [review] XBL fixup I tried it and it fixes one test :). Now one test file gives still errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d5a9736b17f

I add n-i for the last test failure.

Same problem, different place. I'd love to know why this change affects this but I don't want to hold-up this bug for it. Going to try to get through the final review for this today.

Comment on attachment 8590370 [details] [diff] [review] patch Yes, this fixes the failures. bc3 is green now. :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d19859c83d7

Patch with both test fixes from Dave.

diff -w patch

Comment on attachment 8590420 [details] [diff] [review] 989469-useNewCommonStyle.patch Review of attachment 8590420 [details] [diff] [review]: ----------------------------------------------------------------- I still have some concerns over the UI changes here but that could be down to my internal biases and since UX have signed off here it seems best to just get this landed and work from there based on feedback from the community.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d19859c83d7

Backed out for browser_gmpProvider.js failures on OSX and Windows. Also browser_inlinesettings.js on OSX. https://hg.mozilla.org/integration/fx-team/rev/741360778325 https://treeherder.mozilla.org/logviewer.html#?job_id=2638482&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=2638240&repo=fx-team

Really strange. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c67f483424d1 Linux and XP are all green and Win7 has failures in browser_inlinesettings_info.js. What is different between XP and Win7? OS X and also Win7 are looking like they have a problem on Add-on install. I don't think I have changed there something. Dave, sorry to bother you again, please could you look, why this fails on OS X and Win7?

So for the Win7 failures I have a guess at what is going on. The button the test is trying to click is towards the end of a list of options. The style changes make fonts and spacing much larger which could be pushing that button off the bottom of the visible area. Something like |gManagerWindow.document.getElementById("detail-view").ensureElementIsVisible(button)| should solve that. I did come across a bug while looking at that though, the spin buttons for options aren't styles right: https://www.dropbox.com/s/dy8gi1aoapa54dq/Screenshot%202015-04-13%2013.51.53.png?dl=0 I'm still not sure what is going on with the browser_gmpProvider.js test, I can't reproduce the failure on OSX here.

Updated patch to tip. I haven't looked yet at your proposed test fix.

This is what I thought should work but it is probably on the wrong position. See https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7757cc423bb It looks I damaged more than I fixed. Please can you say where the correct position for this one liner is?

(In reply to Dave Townsend [:mossop] from comment #126) > > I did come across a bug while looking at that though, the spin buttons for > options aren't styles right: > https://www.dropbox.com/s/dy8gi1aoapa54dq/Screenshot%202015-04-13%2013.51.53. > png?dl=0 This is something which needs fixing in in-content/common.css. Is this screenshot from a Add-on from the tests?

(In reply to Richard Marti (:Paenglab) from comment #129) > (In reply to Dave Townsend [:mossop] from comment #126) > > > > I did come across a bug while looking at that though, the spin buttons for > > options aren't styles right: > > https://www.dropbox.com/s/dy8gi1aoapa54dq/Screenshot%202015-04-13%2013.51.53. > > png?dl=0 > > This is something which needs fixing in in-content/common.css. Is this > screenshot from a Add-on from the tests? Yes, it's the browser_inlinesettings add-on. (In reply to Richard Marti (:Paenglab) from comment #128) > Created attachment 8592432 [details] [diff] [review] > WinTestFix.diff > > This is what I thought should work but it is probably on the wrong position. > See https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7757cc423bb > > It looks I damaged more than I fixed. Please can you say where the correct > position for this one liner is? You would want it right before it attempts to click the button that is hidden: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js#310

Hmm, I tried to insert the line before http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js#310 and after this failed before http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js#312 and also this fails. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cce7976c11cb and https://treeherder.mozilla.org/#/jobs?repo=try&revision=7306c5fa4975

Comment on attachment 8592062 [details] [diff] [review] 989469-useNewCommonStyle.patch Mike has Windows 7 and is going to take a look at this this this afternoon

Great, thank you Mike. If you have time, please could you also look at the OS X failures in https://treeherder.mozilla.org/#/jobs?repo=try&revision=c67f483424d1 ?

Sorry, never got around to this job during the afternoon. Looking at it now. (In reply to Richard Marti (:Paenglab) from comment #133) > Great, thank you Mike. If you have time, please could you also look at the > OS X failures in > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c67f483424d1 ? Looking...

Update - so far, no luck reproducing this on my Win 7 machine.

So this lets the test pass for me locally. I haven't pushed it to try. We make EventUtils scroll an element into view before synthesizing a click event on its containing window based on its rect. If we take this, do I assume I have to write a similar patch for the other EventUtils.js's sprinkled around the tree?

Comment on attachment 8593517 [details] [diff] [review] 989469.diff Kicked this out to bug 1155324. I'll write up a workaround for Paenglab's patch.

This should do the job.

I made a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e428f6484a8 and I think the issue on Win7 is fixed. I hope the compartment failures are only intermittent as the e10s bc3 is green.

Comment on attachment 8592062 [details] [diff] [review] 989469-useNewCommonStyle.patch Stephen, do you have any idea why these styling changes would break browser_gmpProvider.js on OSX on tinderbox? I can't reproduce locally and I can't really figure out what is going wrong.

Comment on attachment 8592062 [details] [diff] [review] 989469-useNewCommonStyle.patch Oh I think I can see the problem here

Patch with Mike's test fix included.

This test is a little broken, it keeps trying to click items in the plugins list view when the detail view is visible. Adding the assertion to openDetailsView shows a bunch of failures and the rest fix them by switching back to the list view when necessary. I think this was somehow working before the list view was hidden, after it ended up clicking on the homepage of the CDM causing us to load www.openh264.org breaking the rest of the test. That seems to fix the OSX tests but now linux debug tests are generating too many logs, not sure if that is because of this patch or not :( https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8a39c618c0c

Looks like this patch is hitting the assertion in bug 1127198 many many many times. Any thoughts heycam?

Comment on attachment 8594270 [details] [diff] [review] fix browser_gmpProvider.js Review of attachment 8594270 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, aside from the linux failures (of course).

Heycam spotted another UI problem in the current patch, the "Ask to activate" option for Open H264 is disabled but it appears enabled in the current patch: https://www.dropbox.com/s/1ntggqmdeacizdm/Screenshot%202015-04-21%2015.29.31.png?dl=0

(In reply to Dave Townsend [:mossop] from comment #145) > Looks like this patch is hitting the assertion in bug 1127198 many many many > times. Any thoughts heycam? Looking into this in bug 1157097.

(In reply to Dave Townsend [:mossop] from comment #148) > Heycam spotted another UI problem in the current patch, the "Ask to > activate" option for Open H264 is disabled but it appears enabled in the > current patch: > https://www.dropbox.com/s/1ntggqmdeacizdm/Screenshot%202015-04-21%2015.29.31. > png?dl=0 Filed bug 1157143

(In reply to Richard Marti (:Paenglab) from comment #150) > > (In reply to Dave Townsend [:mossop] from comment #148) > > Heycam spotted another UI problem in the current patch, the "Ask to > > activate" option for Open H264 is disabled but it appears enabled in the > > current patch: > > https://www.dropbox.com/s/1ntggqmdeacizdm/Screenshot%202015-04-21%2015.29.31. > > png?dl=0 > > Filed bug 1157143 Thanks. Do we have a bug on file for the problem in comment 126? I'd want to see both those issues fixed before landing this.

(In reply to Dave Townsend [:mossop] from comment #151) > Thanks. Do we have a bug on file for the problem in comment 126? I'd want to > see both those issues fixed before landing this. Not yet. I couldn't reproduce it. I need to look on my OS X VM if this is OS specific.

(In reply to Richard Marti (:Paenglab) from comment #152) > (In reply to Dave Townsend [:mossop] from comment #151) > > > Thanks. Do we have a bug on file for the problem in comment 126? I'd want to > > see both those issues fixed before landing this. > > Not yet. I couldn't reproduce it. I need to look on my OS X VM if this is OS > specific. Filed bug 1157389. I haven't seen it because the needed rules where in preferences.css.

Now that bug 1157097 is fixed (thanks :heycam), Can this be re-landed ?

This needs first a try build with both patches in this bug, but I have now no access to do this now. I can do this this evening.

We also can't land this with the known regressions cause by bug 1157143 and bug 1157389, those will need to be fixed first. I've just reviewed one of them and redirected the review for the other so hopefully we can get this in soon.

I have already started the try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b9ad512c9ee

(In reply to Richard Marti (:Paenglab) from comment #157) > I have already started the try. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b9ad512c9ee The test are looking good. Win8 bc3 failed but a retrigger is green now.

https://hg.mozilla.org/integration/fx-team/rev/83909aa7f4fa https://hg.mozilla.org/integration/fx-team/rev/3b4fa5cc6210

https://hg.mozilla.org/mozilla-central/rev/83909aa7f4fa https://hg.mozilla.org/mozilla-central/rev/3b4fa5cc6210