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

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Toolkit
Application Update
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla1.9.3a3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 wanted)

Details

(URL)

Attachments

(10 attachments, 33 obsolete attachments)

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
rstrong
: review+
Details | Diff | Splinter Review
56.14 KB, patch
rstrong
: 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
Attachment #428392 - Flags: ui-review?(faaborg)
Attachment #428392 - Flags: ui-review?(faaborg) → ui-review+
Created attachment 428403 [details]
screenshot comparison with ka locale (largest width requirement)
OS: Windows Vista → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Comment on attachment 428405 [details] [diff] [review]
patch in progress

This patch requires the patches in bug 530872
Created attachment 428415 [details]
screenshots of several pages

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.
Created attachment 428527 [details]
Possibility for upgrade & update screens, with icon

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.
Created attachment 428588 [details] [diff] [review]
patch in progress
Attachment #428405 - Attachment is obsolete: true

Comment 13

8 years ago
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.
Created attachment 428884 [details]
Mac before / after screenshots

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+
Created attachment 428891 [details]
This type of window on OS X

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
Created attachment 429047 [details]
Mac screenshots

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.
Created attachment 429070 [details]
Mac screenshots

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+
Created attachment 429073 [details]
Windows screenshots

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
Created attachment 429084 [details] [diff] [review]
patch in progress - requires patches from bug 530872
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!
Created attachment 429396 [details] [diff] [review]
1. patch in progress - requires patches from bug 530872
Attachment #429084 - Attachment is obsolete: true
Created attachment 429397 [details] [diff] [review]
2. patch in progress (string changes) - requires patches from bug 530872
Created attachment 429456 [details]
screenshot comparison using fullZoom to zoom out if necessary
Created attachment 429462 [details] [diff] [review]
1. base patch in progress - requires patches from bug 530872
Attachment #429396 - Attachment is obsolete: true
Created attachment 429463 [details] [diff] [review]
2. pinstripe trunk patch in progress - requires patches from bug 530872

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
Created attachment 429464 [details] [diff] [review]
3. patch in progress (string changes for trunk) - requires patches from bug 530872
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.
Created attachment 429932 [details]
Mac screenshots

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
Created attachment 429976 [details]
Windows screenshots
Attachment #428588 - Attachment is obsolete: true
Created attachment 429979 [details] [diff] [review]
1. main patch  - requires patches from bug 530872
Attachment #429462 - Attachment is obsolete: true
Created attachment 429980 [details] [diff] [review]
2. pinstripe trunk patch - requires patches from bug 530872
Attachment #429463 - Attachment is obsolete: true
Created attachment 429981 [details] [diff] [review]
3. string changes for trunk - requires patches from bug 530872
Attachment #429464 - 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.
Created attachment 430236 [details]
Mac screenshot - button fixed
Created attachment 430392 [details]
Windows screenshots - final

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
Created attachment 430887 [details] [diff] [review]
1. main patch without pinstripe changes

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)
Created attachment 430898 [details]
app update test xml
Created attachment 430909 [details]
Ubuntu, Mac, and Win7 screenshots
Attachment #429932 - Attachment is obsolete: true
Attachment #430236 - Attachment is obsolete: true
Attachment #430392 - Attachment is obsolete: true
Created attachment 430916 [details] [diff] [review]
1. main patch without pinstripe changes
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)
Created attachment 430918 [details] [diff] [review]
2. pinstripe trunk patch
Attachment #430918 - Flags: review?(dtownsend)
Created attachment 430919 [details] [diff] [review]
3. string changes for trunk
Attachment #430919 - 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
Created attachment 431006 [details] [diff] [review]
1. main patch without pinstripe changes

made some minor css changes / cleanup
Attachment #430916 - Attachment is obsolete: true
Attachment #431006 - Flags: review?(dtownsend)
Attachment #430916 - Flags: review?(dtownsend)
Created attachment 431008 [details] [diff] [review]
2. pinstripe trunk patch
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+
Created attachment 431994 [details] [diff] [review]
1. main patch without pinstripe changes rev2
Attachment #431006 - Attachment is obsolete: true
Attachment #431994 - Flags: review?(dtownsend)
Created attachment 431995 [details] [diff] [review]
2. pinstripe trunk patch rev2

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!
Created attachment 432000 [details] [diff] [review]
1. main patch without pinstripe changes rev3

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
Last Resolved: 8 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.

Updated

8 years ago
Blocks: 478631
Duplicate of this bug: 478631

Comment 76

8 years ago
Created attachment 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.
(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.

Comment 78

8 years ago
(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

Updated

8 years ago
Depends on: 552563
Depends on: 548764
No longer depends on: 552563

Comment 80

8 years ago
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: 553992
Attachment #432488 - Attachment is obsolete: true
Created attachment 447646 [details]
updated test billboard to handle changes from bug 548061
Attachment #430895 - Attachment is obsolete: true
Created attachment 447647 [details]
Updated app update test xml to handle changes from bug 548061
Attachment #430898 - Attachment is obsolete: true
Depends on: 567258
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: ? → -
status1.9.2: --- → .8-fixed
status1.9.2: .8-fixed → wanted
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
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)
Created attachment 496670 [details] [diff] [review]
1.9.2 patch
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.

Updated

7 years ago
Blocks: 626839
You need to log in before you can comment on or make changes to this bug.