Closed
Bug 480178
Opened 16 years ago
Closed 15 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)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a3
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
()
Details
Attachments
(10 files, 33 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
application/xml
|
Details | |
(deleted),
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.
Comment 1•16 years ago
|
||
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 ?
Assignee | ||
Comment 2•16 years ago
|
||
Will work that out in the process.
Assignee | ||
Comment 3•15 years ago
|
||
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
Assignee | ||
Comment 4•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #428392 -
Flags: ui-review?(faaborg)
Updated•15 years ago
|
Attachment #428392 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Comment 5•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
OS: Windows Vista → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Assignee | ||
Comment 6•15 years ago
|
||
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 428405 [details] [diff] [review]
patch in progress
This patch requires the patches in bug 530872
Assignee | ||
Comment 8•15 years ago
|
||
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
Comment 9•15 years ago
|
||
CC'ing l10n and Pascal for comments.
Assignee | ||
Comment 10•15 years ago
|
||
(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.
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #428405 -
Attachment is obsolete: true
Comment 13•15 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?
Assignee | ||
Comment 14•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
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)
Assignee | ||
Comment 16•15 years ago
|
||
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 17•15 years ago
|
||
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+
Comment 18•15 years ago
|
||
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.
Assignee | ||
Comment 19•15 years ago
|
||
Thanks Alex, I'll clean it up a tad using the margins in that UI as the guide
Assignee | ||
Comment 20•15 years ago
|
||
Alex, new screenshots based on our conversation today.
Attachment #429047 -
Flags: ui-review?(faaborg)
Comment 21•15 years ago
|
||
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+
Assignee | ||
Comment 22•15 years ago
|
||
(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.
Assignee | ||
Comment 23•15 years ago
|
||
(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.
Assignee | ||
Comment 24•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #428884 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #428392 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #429070 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Comment 25•15 years ago
|
||
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 26•15 years ago
|
||
Comment on attachment 429073 [details]
Windows screenshots
no worries, looks good.
Attachment #429073 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Comment 27•15 years ago
|
||
Filed bug 548764 for the non-billboard update found page / finished background download page and bug 548766 for the additional incompatible add-on details
Assignee | ||
Comment 28•15 years ago
|
||
Comment 29•15 years ago
|
||
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.
Assignee | ||
Comment 30•15 years ago
|
||
Hey Markus, can you provide a little background for this?
Comment 31•15 years ago
|
||
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
Assignee | ||
Comment 32•15 years ago
|
||
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!
Assignee | ||
Comment 33•15 years ago
|
||
Attachment #429084 -
Attachment is obsolete: true
Assignee | ||
Comment 34•15 years ago
|
||
Assignee | ||
Comment 35•15 years ago
|
||
Assignee | ||
Comment 36•15 years ago
|
||
Attachment #429396 -
Attachment is obsolete: true
Assignee | ||
Comment 37•15 years ago
|
||
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
Assignee | ||
Comment 38•15 years ago
|
||
Assignee | ||
Comment 39•15 years ago
|
||
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.
Assignee | ||
Comment 40•15 years ago
|
||
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.
Assignee | ||
Comment 41•15 years ago
|
||
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
Assignee | ||
Comment 42•15 years ago
|
||
Attachment #428588 -
Attachment is obsolete: true
Assignee | ||
Comment 43•15 years ago
|
||
Attachment #429462 -
Attachment is obsolete: true
Assignee | ||
Comment 44•15 years ago
|
||
Attachment #429463 -
Attachment is obsolete: true
Assignee | ||
Comment 45•15 years ago
|
||
Attachment #429464 -
Attachment is obsolete: true
Comment 46•15 years ago
|
||
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.
Assignee | ||
Comment 47•15 years ago
|
||
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.
Assignee | ||
Comment 48•15 years ago
|
||
Assignee | ||
Comment 49•15 years ago
|
||
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
Assignee | ||
Comment 50•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #429047 -
Flags: ui-review+
Assignee | ||
Updated•15 years ago
|
Attachment #428392 -
Flags: ui-review+
Assignee | ||
Updated•15 years ago
|
Attachment #428884 -
Flags: ui-review+
Assignee | ||
Comment 51•15 years ago
|
||
Assignee | ||
Comment 52•15 years ago
|
||
Assignee | ||
Comment 53•15 years ago
|
||
Assignee | ||
Comment 54•15 years ago
|
||
Attachment #429932 -
Attachment is obsolete: true
Attachment #430236 -
Attachment is obsolete: true
Attachment #430392 -
Attachment is obsolete: true
Assignee | ||
Comment 55•15 years ago
|
||
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)
Assignee | ||
Comment 56•15 years ago
|
||
Attachment #430918 -
Flags: review?(dtownsend)
Assignee | ||
Comment 57•15 years ago
|
||
Attachment #430919 -
Flags: review?(dtownsend)
Assignee | ||
Comment 58•15 years ago
|
||
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
Assignee | ||
Comment 59•15 years ago
|
||
made some minor css changes / cleanup
Attachment #430916 -
Attachment is obsolete: true
Attachment #431006 -
Flags: review?(dtownsend)
Attachment #430916 -
Flags: review?(dtownsend)
Assignee | ||
Comment 60•15 years ago
|
||
Attachment #430918 -
Attachment is obsolete: true
Attachment #431008 -
Flags: review?(dtownsend)
Attachment #430918 -
Flags: review?(dtownsend)
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 61•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #430919 -
Flags: review?(dtownsend) → review?(dolske)
Assignee | ||
Updated•15 years ago
|
Attachment #431006 -
Flags: review?(dtownsend) → review?(dolske)
Assignee | ||
Updated•15 years ago
|
Attachment #431008 -
Flags: review?(dtownsend) → review?(dolske)
Updated•15 years ago
|
Attachment #431006 -
Flags: review?(dolske) → review-
Comment 62•15 years ago
|
||
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
Updated•15 years ago
|
Attachment #430919 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 63•15 years ago
|
||
(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
Updated•15 years ago
|
Attachment #431008 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 64•15 years ago
|
||
Attachment #431006 -
Attachment is obsolete: true
Attachment #431994 -
Flags: review?(dtownsend)
Assignee | ||
Comment 65•15 years ago
|
||
Moved css to this file... carrying forward r+
Attachment #431995 -
Flags: review+
Assignee | ||
Comment 66•15 years ago
|
||
(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. :(
Comment 67•15 years ago
|
||
(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 68•15 years ago
|
||
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+
Assignee | ||
Comment 69•15 years ago
|
||
bah... I see it now and I'll fix it locally before checking in and resubmit the checked in patch. Thanks!
Assignee | ||
Comment 70•15 years ago
|
||
Fixed indenting... carrying forward r+
Attachment #431008 -
Attachment is obsolete: true
Attachment #431994 -
Attachment is obsolete: true
Attachment #432000 -
Flags: review+
Assignee | ||
Comment 71•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
Assignee | ||
Comment 72•15 years ago
|
||
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-
Comment 73•15 years ago
|
||
Robert, are there any tests we should perform on Litmus?
Assignee | ||
Comment 74•15 years ago
|
||
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.
Comment 76•15 years ago
|
||
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.
Assignee | ||
Comment 77•15 years ago
|
||
(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•15 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.
Assignee | ||
Comment 79•15 years ago
|
||
(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
Assignee | ||
Updated•15 years ago
|
Comment 80•15 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
Assignee | ||
Comment 81•15 years ago
|
||
Good catch Robert... filed bug 553952
Assignee | ||
Updated•15 years ago
|
Attachment #432488 -
Attachment is obsolete: true
Assignee | ||
Comment 82•15 years ago
|
||
Attachment #430895 -
Attachment is obsolete: true
Assignee | ||
Comment 83•15 years ago
|
||
Attachment #430898 -
Attachment is obsolete: true
Assignee | ||
Comment 84•14 years ago
|
||
beltzner, do you still want this on 1.9.2? It is possible to do this without string changes.
blocking1.9.2: --- → ?
Comment 85•14 years ago
|
||
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
Updated•14 years ago
|
Assignee | ||
Comment 86•14 years ago
|
||
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
Assignee | ||
Comment 87•14 years ago
|
||
Assignee | ||
Comment 88•14 years ago
|
||
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)
Assignee | ||
Comment 89•14 years ago
|
||
Attachment #496670 -
Flags: review?(dtownsend)
Assignee | ||
Updated•14 years ago
|
Attachment #496424 -
Attachment is obsolete: true
Attachment #496424 -
Flags: review?(dtownsend)
Comment 90•14 years ago
|
||
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+
Assignee | ||
Comment 91•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•