Closed Bug 480178 Opened 15 years ago Closed 14 years ago

Billboard should extend to available space and the update UI should be the same width for all locales

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3
Tracking Status
blocking1.9.2 --- -
status1.9.2 --- wanted

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

()

Details

Attachments

(10 files, 33 obsolete files)

145.74 KB, image/png
Details
388.78 KB, image/png
Details
775 bytes, application/octet-stream
Details
1.42 MB, image/png
Details
14.73 KB, patch
mossop
: review+
Details | Diff | Splinter Review
7.99 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
56.14 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
12.53 KB, text/html
Details
563 bytes, application/xml
Details
34.38 KB, patch
mossop
: review+
Details | Diff | Splinter Review
Instead of a page that has a browser element, change the update snippet format to include references to resources (images, text) which will be assembled in a XUL dialog on the client side.
How would this work if you wanted to change some of those resources ? New snippets generated and tested ? It's a great deal more work to validate that than using a single html page as we do now. Could the snippet point to a manifest of resources on the network ?
Will work that out in the process.
I spent some time looking at the update ui on different locales and don't think there is much value in removing the browser element... the main thing being discussed to be solved by this bug was the difficulty in verifying that each locale's billboard fit properly in the UI and the same problem would exist even without the browser element. I also checked the widths for the update ui for each locale with a value different than en-US and think we can have a single width of the update UI for Windows / Linux and Mac for all locales. This would reduce the verification significantly in that the html for each locale billboard would have the same width and height available to them.

Another thing that has come up several times previously is extending the billboard to the available space which I believe I have working adequately.

Morphing bug...
Summary: Rebuild Software Update without browser element → Billboard should extend to available space and the update UI should be the same width for all locales
Attached image screenshot (obsolete) —
Attachment #428392 - Flags: ui-review?(faaborg)
Attachment #428392 - Flags: ui-review?(faaborg) → ui-review+
OS: Windows Vista → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Attached patch patch in progress (obsolete) — Splinter Review
Comment on attachment 428405 [details] [diff] [review]
patch in progress

This patch requires the patches in bug 530872
Attached image screenshots of several pages (obsolete) —
I changed the sizing a little but to get rid of the excess space to the left and right of the wizard content. I'd like to come up with better ui for the non billboard case and the update has already been downloaded pages on the right of the screenshots
CC'ing l10n and Pascal for comments.
(In reply to comment #9)
> CC'ing l10n and Pascal for comments.
For reference: the l10n details are in comment #3... basically, the update ui would have the same width for all locales and it would be the largest width as the current largest width specified by a locale minus the width requirement reduction made by changing the padding on the buttons to the default padding for them.
You could handle the two right screens in the diagram by showing an actual icon for what's going to be installed.  That has the dual benefit of taking up the window space a bit more fully, and making it more clear what the user is about to get or install.
Attached patch patch in progress (obsolete) — Splinter Review
Attachment #428405 - Attachment is obsolete: true
How would we find out if we had to increase the width of the dialog when a new locale comes in?

I didn't fully understand what the width is depending on, is button labels the criteria that matters here?
The buttons along the bottom can be hidden / shown based on the update xml and the labels in some locales require a larger width than the wizard default... this has been the case ever since the billboard was added.

As it stands today we just had a bunch of locales that weren't wide enough due to the 'No Thanks' button being hidden except for major updates. Besides going with a width that satisfies all current locales I'd like to just provide a url that localizers can use to test the ui with by setting a pref. The update offered by this url wouldn't affect the build they are testing the ui with in any way. This wouldn't be able to test the billboard since the billboard can change but it shouldn't be necessary since the dimensions would be the same for all locales. Also, if someone wants to see whether the billboard fits properly it should be possible to do so with a javascript bookmarklet to resize the window.
Attached image Mac before / after screenshots (obsolete) —
Alex, I finished my first go at the pages for Mac OS X. I went with the default margins for the buttons which seems a tad large even for Mac but it is consistent with other parts of the Firefox UI though it doesn't seem that consistent so if you'd like that or anything else changed please let me know.
Attachment #428884 - Flags: ui-review?(faaborg)
Comment on attachment 428884 [details]
Mac before / after screenshots

btw: these are before and after screenshots
Attachment #428884 - Attachment description: Mac screenshots → Mac before / after screenshots
Comment on attachment 428884 [details]
Mac before / after screenshots

Better than what we have, we can work on improving it in follow up bugs.
Attachment #428884 - Flags: ui-review?(faaborg) → ui-review+
Here is a window on OS X that matches the general layout and purpose, and uses an approach similar to the unified toolbar, except on the bottom.
Thanks Alex, I'll clean it up a tad using the margins in that UI as the guide
Attached image Mac screenshots (obsolete) —
Alex, new screenshots based on our conversation today.
Attachment #429047 - Flags: ui-review?(faaborg)
Comment on attachment 429047 [details]
Mac screenshots

awesome :) only two small nits:

-perhaps make the buttons 22px tall?

-not sure if we can use bold on a button like this, looks out of place for some reason (although I might be wrong).
Attachment #429047 - Flags: ui-review?(faaborg) → ui-review+
(In reply to comment #21)
> (From update of attachment 429047 [details])
> awesome :) only two small nits:
> 
> -perhaps make the buttons 22px tall?
Will do using px to increase the padding of the button.

> -not sure if we can use bold on a button like this, looks out of place for some
> reason (although I might be wrong).
I felt the same way about it on Windows... it was something I was asked to add awhile ago and on Mac it was missing a required !important for it to take affect. If it is removed I'd like to remove it from Windows as well.
(In reply to comment #22)
> (In reply to comment #21)
>...
> > -not sure if we can use bold on a button like this, looks out of place for some
> > reason (although I might be wrong).
> I felt the same way about it on Windows... it was something I was asked to add
> awhile ago and on Mac it was missing a required !important for it to take
> affect. If it is removed I'd like to remove it from Windows as well.
In case it isn't clear why I don't like using bold text to draw attention to a button especially on Windows the default / important button is highlighted and doesn't really need to be bold. I could see this being necessary on Mac perhaps since there isn't any visual indication of which button is the default / important.
Attached image Mac screenshots (obsolete) —
I also left the default wizard button width since the OK button was such a small target and looked rather funky. If possible, any other changes I'd prefer to do in a followup bug
Attachment #429047 - Attachment is obsolete: true
Attachment #429070 - Flags: ui-review?(faaborg)
Attachment #428884 - Attachment is obsolete: true
Attachment #428392 - Attachment is obsolete: true
Attachment #429070 - Flags: ui-review?(faaborg) → ui-review+
Attached image Windows screenshots (obsolete) —
Alex, sorry about all of the reviews on this... here are the Windows screenshots. I'll file a followup for fixing the two pages on the right for all platforms and another bug on providing more details for add-ons in the Incompatible Add-ons page
Attachment #429073 - Flags: ui-review?(faaborg)
Comment on attachment 429073 [details]
Windows screenshots

no worries, looks good.
Attachment #429073 - Flags: ui-review?(faaborg) → ui-review+
Filed bug 548764 for the non-billboard update found page / finished background download page and bug 548766 for the additional incompatible add-on details
For the pinstripe styles on the buttons please use file preprocessing like we do for example for console.css. I think these buttons need the same code as the #Console\:clear button over there.
Hey Markus, can you provide a little background for this?
Sure.
In order to avoid mistakes when repeating the same styles in different files, in bug 508940 we added the file toolkit/themes/pinstripe/global/shared.inc and made use of the text preprocessor [1] that we also use for making %ifdefs in CSS files work.

When you want to make use of shared.inc in your CSS file you need to
1. ensure that your file is run through the preprocessor at build time by adding an asterisk in front of the file's line in jar.mn [2] and
2. add the line %include "shared.inc" (or in your case probably "../../global/shared.inc") somewhere in the file.

Then you can replace for example this line
-moz-box-shadow: inset rgba(0, 0, 0, 0.4) 0 -5px 12px, inset rgba(0, 0, 0, 1) 0 1px 3px, rgba(255, 255, 255, 0.4) 0 1px;
with
-moz-box-shadow: @toolbarbuttonPressedInnerShadow@, @loweredShadow@;

The @@ syntax is turned on by "%filter substitution" in shared.inc.

[1] https://developer.mozilla.org/en/Build/Text_Preprocessor
[2] https://developer.mozilla.org/en/JAR_Manifests#Shipping_Chrome_Files
Thanks Markus... I used the download manager to get the button css which didn't use preprocessing for the button style and hadn't realized this had been added. Glad to hear it!
I was developing with 1.9.2 on the Mac since my Mac can't run trunk which is why I missed the include in pinstripe being used on trunk.
Attachment #429397 - Attachment is obsolete: true
There have been additional changes requested that require additional overrides to wizard.xml. With so damn many overrides already I am going to take wizard.xml and make it specific to the update UI instead of adding more overrides wizard.xml so I can avoid having to fix the bugs that tend to occur due to overriding expected behavior and so it is easier to make future changes that would likely also require overriding existing wizard.xml behavior.
Note for l10n people: UX wants to go with a 16x10 aspect ratio for this UI. This will make the width significantly larger than the minimum width required for the locale with the largest width requirement.
Attached image Mac screenshots (obsolete) —
faaborg asked me to change the window size on all platforms to a ratio of 16 x 10
Attachment #428403 - Attachment is obsolete: true
Attachment #428415 - Attachment is obsolete: true
Attachment #429070 - Attachment is obsolete: true
Attachment #429073 - Attachment is obsolete: true
Attachment #429456 - Attachment is obsolete: true
Attached image Windows screenshots (obsolete) —
Attachment #428588 - Attachment is obsolete: true
Comment on attachment 429932 [details]
Mac screenshots

The font size of the next / finish button is wrong (too small).
I'm also not sure about using bold here - maybe we should just get rid of it and hope that the button's position makes it clear enough that it's a default button. I haven't seen any native Mac apps that use bold text on this button type.
I saw that as well though I didn't change the font size... I'll investigate tomorrow.

I agree that we shouldn't be using bold for button label text but I had this discussion a couple of years ago with beltzner and he wanted it. On Windows the default button - which this button is - also has a blue glow to draw attention to it. I'll bring it up again tomorrow to see if he has changed his mind.
Attached image Windows screenshots - final (obsolete) —
Only includes a couple of the screens since the layout on the rest is the same. This is going to be the final revision and additional tweaks can be dealt with in other bugs such as bug 548764 which I'll likely morph to cover all additional tweaks.
Attachment #429976 - Attachment is obsolete: true
I've put the changes to pinstripe in a separate patch since it will be significantly different when backported due to 1.9.2 not having shared.inc for the styling... I'll attach the remaining patches after I've had a chance to clean up the button spacing on pinstripe.
Attachment #429979 - Attachment is obsolete: true
Attachment #430887 - Flags: review?(dtownsend)
Attachment #429047 - Flags: ui-review+
Attachment #428392 - Flags: ui-review+
Attachment #428884 - Flags: ui-review+
Attached file test billboard (obsolete) —
Attached file app update test xml (obsolete) —
Attachment #429932 - Attachment is obsolete: true
Attachment #430236 - Attachment is obsolete: true
Attachment #430392 - Attachment is obsolete: true
Attachment #429980 - Attachment is obsolete: true
Attachment #429981 - Attachment is obsolete: true
Attachment #430887 - Attachment is obsolete: true
Attachment #430916 - Flags: review?(dtownsend)
Attachment #430887 - Flags: review?(dtownsend)
Attached patch 2. pinstripe trunk patch (obsolete) — Splinter Review
Attachment #430918 - Flags: review?(dtownsend)
To test the patches you can add a new string pref name app.update.url.override with a value of https://bugzilla.mozilla.org/attachment.cgi?id=430898
made some minor css changes / cleanup
Attachment #430916 - Attachment is obsolete: true
Attachment #431006 - Flags: review?(dtownsend)
Attachment #430916 - Flags: review?(dtownsend)
Attached patch 2. pinstripe trunk patch (obsolete) — Splinter Review
Attachment #430918 - Attachment is obsolete: true
Attachment #431008 - Flags: review?(dtownsend)
Attachment #430918 - Flags: review?(dtownsend)
Comment on attachment 431006 [details] [diff] [review]
1. main patch without pinstripe changes

>diff --git a/toolkit/mozapps/update/content/updates.css b/toolkit/mozapps/update/content/updates.css
>--- a/toolkit/mozapps/update/content/updates.css
>+++ b/toolkit/mozapps/update/content/updates.css
>@@ -1,34 +1,55 @@
>-/**
>- * General
>- */
>-wizard[description=""] .wizard-header-description {
>-  display: none;
>+/* Specify the size for the wizardpage so the billboard has a fixed size. */
>+wizardpage {
>+  height: 360px;
>+  width: 700px;
> }
I think it would be best to put this in the theme css with the exception of when backporting this change since themes on previous releases won't have this.
Attachment #430919 - Flags: review?(dtownsend) → review?(dolske)
Attachment #431006 - Flags: review?(dtownsend) → review?(dolske)
Attachment #431008 - Flags: review?(dtownsend) → review?(dolske)
Attachment #431006 - Flags: review?(dolske) → review-
Comment on attachment 431006 [details] [diff] [review]
1. main patch without pinstripe changes

Couple of brief things that need answers here:

>diff --git a/toolkit/mozapps/update/content/updates.css b/toolkit/mozapps/update/content/updates.css
>--- a/toolkit/mozapps/update/content/updates.css
>+++ b/toolkit/mozapps/update/content/updates.css
>@@ -1,34 +1,55 @@
>-/**
>- * General
>- */
>-wizard[description=""] .wizard-header-description {
>-  display: none;
>+/* Specify the size for the wizardpage so the billboard has a fixed size. */
>+wizardpage {
>+  height: 360px;
>+  width: 700px;
> }
> 
>-/**
>- * Stop animations when they aren't displayed (bug 341749).
>- */
>+/* Remove margin and padding so the billboard will extend to the edge of the
>+   window */
>+#updates, .wizard-page-box {
>+  margin: 0;
>+  padding: 0;
>+}

These feel like things that should be in the theme, not the content styles.

>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js

>+    // Provide the ability to test the billboard html
>+    var billboardTestURL = getPref("getCharPref", PREF_APP_UPDATE_BILLBOARD_TEST_URL, null);
>+    if (billboardTestURL) {
>+      var updatesFoundBillboardPage = document.getElementById("updatesfoundbillboard");
>+      updatesFoundBillboardPage.setAttribute("next", "dummy");
>+      gUpdatesFoundBillboardPage.onExtra1 = function(){ gUpdates.wiz.cancel(); };
>+      gUpdatesFoundBillboardPage.onExtra2 = function(){ gUpdates.wiz.cancel(); };
>+      this.onWizardNext = function() { gUpdates.wiz.cancel(); };
>+      this.update = { billboardURL        : billboardTestURL,
>+                      brandName           : this.brandName,
>+                      displayVersion      : "Billboard Test 1.0",
>+                      showNeverForVersion : true,
>+                      type                : "major" };
>+      return updatesFoundBillboardPage;
>+    }
>+

>-    var introText = gUpdates.getAUSString("intro_" + update.type,
>-                                          [gUpdates.brandName, update.displayVersion]);
>-    var introElem = document.getElementById("updatesFoundBillboardIntro");
>-    introElem.setAttribute("severity", update.type);
>-    introElem.textContent = introText;
>+    // Allow file urls when testing the billboard

This seem to actually be "Only allow file urls when testing the billboard"

>+    var billboardTestURL = getPref("getCharPref", PREF_APP_UPDATE_BILLBOARD_TEST_URL, null);
>+    if (billboardTestURL) {
>+      var ioServ = CoC["@mozilla.org/network/io-service;1"].
>+               getService(CoI.nsIIOService);
>+      var scheme = ioServ.newURI(billboardTestURL, null, null).scheme;
>+      if (scheme && scheme == "file")

Just (scheme == "file") should suffice.

>diff --git a/toolkit/mozapps/update/content/updates.xml b/toolkit/mozapps/update/content/updates.xml

>       <method name="onLoad">
>         <body><![CDATA[
>+          // The remote html can tell us to zoom out the page if the page
>+          // will create scrollbars by adding one of the following attributes.
>+          // Since zooming out will not always remove the scrollbars even though
>+          // the page fits in the client region after zooming out the web page
>+          // should hide the scrollbar(s) using css.
>+          // zoomOutToFit: zoom out if the width or height is greater than the
>+          //               client region.
>+          // zoomOutToFitHeight: zoom out if the height is greater than the
>+          //                     client region.
>+          // zoomOutToFitWidth: zoom out if the width is greater than the
>+          //                    client region.

I think we should just use a single attribute here, zoomToFit="both" or "width" or "height". Maybe -moz-zoom as it is a proprietary attribute. Another (harder I guess) alternative would be to use the standard viewport meta tag to achieve the same result, but let's leave that.

>+  <binding id="updateheader" extends="chrome://global/content/bindings/wizard.xml#wizardpage">

This doesn't seem right, maybe it should extent wizard-header?

But can't you instead actually do something that extends wizardpage, make all of the pages use it and put the header you want in there followed by the vbox with all the children inside it rather than the changes throughout updates.xul

>diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul

>+  <wizardpage id="incompatibleCheck" pageid="incompatibleCheck"
>+              next="updatesfoundbasic" object="gIncompatibleCheckPage"
>+              onpageshow="gIncompatibleCheckPage.onPageShow();">
>+    <updateheader label="&incompatibleCheck.title;"/>
>+      <vbox class="update-content" flex="1">

Some bad indenting here
Attachment #430919 - Flags: review?(dolske) → review+
(In reply to comment #62)
> (From update of attachment 431006 [details] [diff] [review])
> Couple of brief things that need answers here:
> 
> >diff --git a/toolkit/mozapps/update/content/updates.css b/toolkit/mozapps/update/content/updates.css
> >--- a/toolkit/mozapps/update/content/updates.css
> >+++ b/toolkit/mozapps/update/content/updates.css
> >@@ -1,34 +1,55 @@
> >-/**
> >- * General
> >- */
> >-wizard[description=""] .wizard-header-description {
> >-  display: none;
> >+/* Specify the size for the wizardpage so the billboard has a fixed size. */
> >+wizardpage {
> >+  height: 360px;
> >+  width: 700px;
> > }
> > 
> >-/**
> >- * Stop animations when they aren't displayed (bug 341749).
> >- */
> >+/* Remove margin and padding so the billboard will extend to the edge of the
> >+   window */
> >+#updates, .wizard-page-box {
> >+  margin: 0;
> >+  padding: 0;
> >+}
> 
> These feel like things that should be in the theme, not the content styles.
I agree except for when backporting these changes since it will break 3rd party themes as stated below... how about just on trunk?

(In reply to comment #61)
> (From update of attachment 431006 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/update/content/updates.css b/toolkit/mozapps/update/content/updates.css
> >--- a/toolkit/mozapps/update/content/updates.css
> >+++ b/toolkit/mozapps/update/content/updates.css
> >@@ -1,34 +1,55 @@
> >-/**
> >- * General
> >- */
> >-wizard[description=""] .wizard-header-description {
> >-  display: none;
> >+/* Specify the size for the wizardpage so the billboard has a fixed size. */
> >+wizardpage {
> >+  height: 360px;
> >+  width: 700px;
> > }
> I think it would be best to put this in the theme css with the exception of
> when backporting this change since themes on previous releases won't have this.

> >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
> 
> >+    // Provide the ability to test the billboard html
> >+    var billboardTestURL = getPref("getCharPref", PREF_APP_UPDATE_BILLBOARD_TEST_URL, null);
> >+    if (billboardTestURL) {
> >+      var updatesFoundBillboardPage = document.getElementById("updatesfoundbillboard");
> >+      updatesFoundBillboardPage.setAttribute("next", "dummy");
> >+      gUpdatesFoundBillboardPage.onExtra1 = function(){ gUpdates.wiz.cancel(); };
> >+      gUpdatesFoundBillboardPage.onExtra2 = function(){ gUpdates.wiz.cancel(); };
> >+      this.onWizardNext = function() { gUpdates.wiz.cancel(); };
> >+      this.update = { billboardURL        : billboardTestURL,
> >+                      brandName           : this.brandName,
> >+                      displayVersion      : "Billboard Test 1.0",
> >+                      showNeverForVersion : true,
> >+                      type                : "major" };
> >+      return updatesFoundBillboardPage;
> >+    }
> >+
> 
> >-    var introText = gUpdates.getAUSString("intro_" + update.type,
> >-                                          [gUpdates.brandName, update.displayVersion]);
> >-    var introElem = document.getElementById("updatesFoundBillboardIntro");
> >-    introElem.setAttribute("severity", update.type);
> >-    introElem.textContent = introText;
> >+    // Allow file urls when testing the billboard
> 
> This seem to actually be "Only allow file urls when testing the billboard"
If the scheme isn't file then it fallsback to the original method which allows testing http(s), etc.

> >+    var billboardTestURL = getPref("getCharPref", PREF_APP_UPDATE_BILLBOARD_TEST_URL, null);
> >+    if (billboardTestURL) {
> >+      var ioServ = CoC["@mozilla.org/network/io-service;1"].
> >+               getService(CoI.nsIIOService);
> >+      var scheme = ioServ.newURI(billboardTestURL, null, null).scheme;
> >+      if (scheme && scheme == "file")
> 
> Just (scheme == "file") should suffice.
Will do

> >diff --git a/toolkit/mozapps/update/content/updates.xml b/toolkit/mozapps/update/content/updates.xml
> 
> >       <method name="onLoad">
> >         <body><![CDATA[
> >+          // The remote html can tell us to zoom out the page if the page
> >+          // will create scrollbars by adding one of the following attributes.
> >+          // Since zooming out will not always remove the scrollbars even though
> >+          // the page fits in the client region after zooming out the web page
> >+          // should hide the scrollbar(s) using css.
> >+          // zoomOutToFit: zoom out if the width or height is greater than the
> >+          //               client region.
> >+          // zoomOutToFitHeight: zoom out if the height is greater than the
> >+          //                     client region.
> >+          // zoomOutToFitWidth: zoom out if the width is greater than the
> >+          //                    client region.
> 
> I think we should just use a single attribute here, zoomToFit="both" or "width"
> or "height". Maybe -moz-zoom as it is a proprietary attribute. Another (harder
> I guess) alternative would be to use the standard viewport meta tag to achieve
> the same result, but let's leave that.
WIll do

> >+  <binding id="updateheader" extends="chrome://global/content/bindings/wizard.xml#wizardpage">
> 
> This doesn't seem right, maybe it should extent wizard-header?
I'll look into it.

> But can't you instead actually do something that extends wizardpage, make all
> of the pages use it and put the header you want in there followed by the vbox
> with all the children inside it rather than the changes throughout updates.xul
Nope, I need the billboard page to not have one so the window sizes correctly on creation.

> >diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul
> 
> >+  <wizardpage id="incompatibleCheck" pageid="incompatibleCheck"
> >+              next="updatesfoundbasic" object="gIncompatibleCheckPage"
> >+              onpageshow="gIncompatibleCheckPage.onPageShow();">
> >+    <updateheader label="&incompatibleCheck.title;"/>
> >+      <vbox class="update-content" flex="1">
> 
> Some bad indenting here
Thanks
Attachment #431008 - Flags: review?(dolske) → review+
Attachment #431006 - Attachment is obsolete: true
Attachment #431994 - Flags: review?(dtownsend)
Moved css to this file... carrying forward r+
Attachment #431995 - Flags: review+
(In reply to comment #62)
> (From update of attachment 431006 [details] [diff] [review])
> Couple of brief things that need answers here:
> >diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul
> 
> >+  <wizardpage id="incompatibleCheck" pageid="incompatibleCheck"
> >+              next="updatesfoundbasic" object="gIncompatibleCheckPage"
> >+              onpageshow="gIncompatibleCheckPage.onPageShow();">
> >+    <updateheader label="&incompatibleCheck.title;"/>
> >+      <vbox class="update-content" flex="1">
> 
> Some bad indenting here
I don't see it. :(
(In reply to comment #63)
> (In reply to comment #62)
> > (From update of attachment 431006 [details] [diff] [review] [details])
> > Couple of brief things that need answers here:
> > 
> > >diff --git a/toolkit/mozapps/update/content/updates.css b/toolkit/mozapps/update/content/updates.css
> > >--- a/toolkit/mozapps/update/content/updates.css
> > >+++ b/toolkit/mozapps/update/content/updates.css
> > >@@ -1,34 +1,55 @@
> > >-/**
> > >- * General
> > >- */
> > >-wizard[description=""] .wizard-header-description {
> > >-  display: none;
> > >+/* Specify the size for the wizardpage so the billboard has a fixed size. */
> > >+wizardpage {
> > >+  height: 360px;
> > >+  width: 700px;
> > > }
> > > 
> > >-/**
> > >- * Stop animations when they aren't displayed (bug 341749).
> > >- */
> > >+/* Remove margin and padding so the billboard will extend to the edge of the
> > >+   window */
> > >+#updates, .wizard-page-box {
> > >+  margin: 0;
> > >+  padding: 0;
> > >+}
> > 
> > These feel like things that should be in the theme, not the content styles.
> I agree except for when backporting these changes since it will break 3rd party
> themes as stated below... how about just on trunk?

Sounds good.
Comment on attachment 431994 [details] [diff] [review]
1. main patch without pinstripe changes rev2

r=me though bugzilla is still showing the opening <vbox> indented too far in the incompatibleCheck page.
Attachment #431994 - Flags: review?(dtownsend) → review+
bah... I see it now and I'll fix it locally before checking in and resubmit the checked in patch. Thanks!
Fixed indenting... carrying forward r+
Attachment #431008 - Attachment is obsolete: true
Attachment #431994 - Attachment is obsolete: true
Attachment #432000 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/58de25578e9b
http://hg.mozilla.org/mozilla-central/rev/46d0616704ef
http://hg.mozilla.org/mozilla-central/rev/c4ff7536bc71
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
There are mochitests chrome tests already for the billboard but not to test the sizing across all locales, etc. since we don't run tests on l10n so minus in-testsuite
Flags: in-testsuite-
Robert, are there any tests we should perform on Litmus?
I don't think so. The sizing needs to be checked when the html is created and now that it is a fixed size that is much simpler to do. The verification that there is a billboard, the billboard loads, etc. when the update xml states there is a billboard are already tested in several mochitest chrome tests that also test a bunch of other things.
Blocks: 478631
Minefield EN_US version 3.7a3pre(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a3pre) Gecko/20100314 Minefield/3.7a3pre (.NET CLR 3.5.30729) on Windows XP SP2 Japanese)

get a very wide size of Update status windows.
(In reply to comment #76)
> Created an attachment (id=432488) [details]
> Minefield EN_US version 3.7a3pre(Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9.3a3pre) Gecko/20100314 Minefield/3.7a3pre (.NET CLR 3.5.30729) on
> Windows XP SP2 Japanese)
> 
> Minefield EN_US version 3.7a3pre(Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9.3a3pre) Gecko/20100314 Minefield/3.7a3pre (.NET CLR 3.5.30729) on
> Windows XP SP2 Japanese)
> 
> get a very wide size of Update status windows.
Try setting your theme to the default theme and try again.
Depends on: 552371
(In reply to comment #77)
> > get a very wide size of Update status windows.
> Try setting your theme to the default theme and try again.
I'm using the default theme along with at least a few people on the mozillazine forums and we have the same problem.
(In reply to comment #78)
> (In reply to comment #77)
> > > get a very wide size of Update status windows.
> > Try setting your theme to the default theme and try again.
> I'm using the default theme along with at least a few people on the mozillazine
> forums and we have the same problem.
The screenshot that was attached for that comment was obviously not the default theme.

If you are experiencing this with the default theme please file a new bug with as much insight as you can provide to help figure out this issue and a screenshot. Thanks
Depends on: 552563
Depends on: 548764
No longer depends on: 552563
Comment on attachment 432000 [details] [diff] [review]
1. main patch without pinstripe changes rev3

>--- a/toolkit/themes/winstripe/mozapps/jar.mn
>+++ b/toolkit/themes/winstripe/mozapps/jar.mn
>-        skin/classic/mozapps/update/update.png                     (update/update.png)

>deleted file mode 100644
>Binary file toolkit/themes/winstripe/mozapps/update/update.png has changed

I just noticed that this and other similar changes removed the file that is still in use by this line of theme CSS:
http://hg.mozilla.org/mozilla-central/file/tip/toolkit/themes/winstripe/mozapps/extensions/update.css#l2
Good catch Robert... filed bug 553952
Depends on: 553952
Attachment #432488 - Attachment is obsolete: true
beltzner, do you still want this on 1.9.2? It is possible to do this without string changes.
blocking1.9.2: --- → ?
Not going to "block" a release on the stable branch. At some point we may want it.
blocking1.9.2: ? → -
I would like this on 1.9.2 but only if other fixes are backported to 1.9.2 which is bug 576939 mainly because there are app update ui tests so adding dependency.
Depends on: 576939
No longer blocks: 478631
Attached patch 1.9.2 patch (obsolete) — Splinter Review
Comment on attachment 496424 [details] [diff] [review]
1.9.2 patch

Dave, would you mind taking a look at this? It updates the code to be essentially the same as on trunk
Attachment #496424 - Flags: review?(dtownsend)
Attached patch 1.9.2 patchSplinter Review
Attachment #496670 - Flags: review?(dtownsend)
Attachment #496424 - Attachment is obsolete: true
Attachment #496424 - Flags: review?(dtownsend)
Comment on attachment 496670 [details] [diff] [review]
1.9.2 patch

I'm a little concerned at how much UI change there is here that may impact extensions compatibility, but I guess it is limited to one window that extensions will rarely be touching. MR Tech Toolkit looks like the only one on AMO that overlays the update dialog, does it break with this patch applied?
Attachment #496670 - Flags: review?(dtownsend) → review+
(In reply to comment #90)
> Comment on attachment 496670 [details] [diff] [review]
> 1.9.2 patch
> 
> I'm a little concerned at how much UI change there is here that may impact
> extensions compatibility, but I guess it is limited to one window that
> extensions will rarely be touching. MR Tech Toolkit looks like the only one on
> AMO that overlays the update dialog, does it break with this patch applied?
Short answer is no, it doesn't break the app update ui.

Long answer:
Just took a look at Mr. Tech and the extension doesn't overlay the updates.xul properly.

From Mr. Tech's chrome.manifest
overlay	chrome://mozapps/content/update/update.xul	chrome://local_install/content/local_updates.xul

That should be chrome://mozapps/content/update/updates.xul

Also, it has the following in local_updates.xul

<overlay id="local_installUpdatesOverlay" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

  <script type="application/x-javascript" src="chrome://global/content/nsUserSettings.js"/>
  <script type="application/x-javascript" src="chrome://local_install/content/mrtech-common.js"/>
  <script type="application/x-javascript" src="chrome://local_install/content/branding.js"/>
  <script type="application/x-javascript" src="chrome://local_install/content/local_install.js"/>

  <wizard id="updateWizard"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
        title="&updateWizard.title;"
        windowtype="Update:Wizard"
        onload="gUpdateWizard.init(); myUpdatePage.init();"
        onunload="gUpdateWizard.uninit(); myUpdatePage.uninit();"
        onwizardfinish="gUpdateWizard.onWizardFinish();"
        onclose="return gUpdateWizard.onWizardClose(event);"
        style="width: 47em; min-height: 35em;"
        buttons="accept,cancel" />

</overlay>

I was unable to find myUpdatePage anywhere in any of Mr. Tech's files so it is a good thing it isn't overlaying the app update ui.
Blocks: 626839
You need to log in before you can comment on or make changes to this bug.