1247900 - about:newaddon page has ugly table cell design

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[arni2033]

Attachment: screenshot 1 - about-newaddon page has ugly table design.png

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160210071115
STR:
1. Install https://addons.mozilla.org/ru/firefox/addon/multiple-tab-handler/reviews/
2. Close the browser, create new profile, close that profile.
3. Copy the extension "multiple tab handler" from the profile in Step 1
   to the /extensions/ folder in the profile created in Step 2
4. Launch the profile created in Step 2
5. Make sure about:newaddon page opened. Check "Allow this installation", click "Continue"
6. Open devtools -> Inspector.
7. Find element #restartMessage (by clicking on dropmarkers and pressing Up and Down arrows - or
   you will run into bug 1228675).
8. Replace innerHTML of the element found in Step 6
   from "You must restart Nightly to finish installing this add-on."
   to   "Для завершения установки этого дополнения вы должны перезапустить Firefox Developer Edition"
9. Find element #restart-button (it's the next sibling actually)
10. Replace its "label" attribute from "Restart Nightly" to "Перезапустить Firefox Developer Edition"

AR:  The text in  #restartMessage  looks very bad and is difficult to read.
     That's how it looks in my localization.
ER:  That text shouldn't be aligned vertically.

Comment 1

[Tony Mechelynck [:tonymec]]

For once, Google translate seems to provide a decent translation for the Russian text at step 8, namely, "To complete the installation of the add-ons you need to restart Firefox Developer Edition".

Comment 2

[Tony Mechelynck [:tonymec]]

It isn't html:tr design, it's xul:hbox design but the result is the same. Here's the responsible snippet (in https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/newaddon.xul):

55         <hbox id="restartPanel">
56           <spacer id="restartSpacer"/>
57           <description id="restartMessage" flex="1">&restartMessage;</description>
58           <button id="restart-button" label="&restartButton;" oncommand="restartClicked()"/>
59           <button id="cancel-button" label="&cancelButton;" oncommand="cancelClicked()"/>
60         </hbox>

Of course, the comparative verbosity of Russian makes the problem particularly acute in that language.

Comment 3

[Tony Mechelynck [:tonymec]]

Attachment: rearrange contents of newaddon.xul#restartPanel

Not sure this is acceptable but it's all I found. If the spacer can really go away (please tell me) I'll remove its commented-out entry.

Comment 4

[arni2033]

Just a note. Your patch is likely to cause button twitching after click. I.e.:
Currently        when I click "Continue", button "Cancel" appears right under the mouse pointer.
After your patch when I click "Continue", button "Cancel" will appear ~40px below the mouse pointer.
  
I saw this in private tabs, when I click "enable/disable tracking protection". This is undesirable (IMO), but OFC not so awful as cases when element moves away from mouse pointer ~0.1s _before_ click

Comment 5

[Tony Mechelynck [:tonymec]]

(In reply to arni2033 from comment #4)
> Just a note. Your patch is likely to cause button twitching after click.
> I.e.:
> Currently        when I click "Continue", button "Cancel" appears right
> under the mouse pointer.
> After your patch when I click "Continue", button "Cancel" will appear ~40px
> below the mouse pointer.
>   
> I saw this in private tabs, when I click "enable/disable tracking
> protection". This is undesirable (IMO), but OFC not so awful as cases when
> element moves away from mouse pointer ~0.1s _before_ click

Maybe the spacer should remain then. Let's see what Dave says. (I don't have anything against an r- if it says in detail _why_ review wasn't granted. In fact I prefer an r- which learns me something to an empty r+.)

Comment 6

[Tony Mechelynck [:tonymec]]

Or else, a spacer and/or some text should be added in the other element of the <deck>, above #continue-button

Comment 7

[Tony Mechelynck [:tonymec]]

(In reply to arni2033 from comment #4) (continued)
It might also be a case of "You can't have your cake and eat it too". The way it is presently, with the text and both buttons arrayed horizontally, you don't have to move your mouse, but then the text is horribly cramped, especially in Russian where it is longer than in English. By putting the text above the buttons, it flows more naturally, but then you have to move your mouse after hitting Continue. If I added two line of bogus text ("Lorem ipsum" or such, see https://en.wikipedia.org/wiki/Lorem_ipsum) above the Continue button, you would again not have to move your mouse, but then the bogus text would make it unacceptably ugly. One more possibility would be to increase the height of the Continue button to make it as high as the "You need to restart" text plus both buttons, but then after clicking near the centre of the resized Continue button your mouse pointer would still not be in the right place for the Cancel button.

So what is more important? "Deuglifying" the label or avoiding to move the mouse? I believe that ugly text is much less acceptable than having to move the mouse between clicks; but again, let's hear what Mossop has to say.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8719185 [details] [diff] [review]
rearrange contents of newaddon.xul#restartPanel

I think if you play with the align and pack attributes on the deck then it would snap the contents to the bottom so they stay in the same place when switching. I think align="end" is what you'll need.

If you could upload a screenshot when it is ready for review that would help a lot.

Thanks.

Comment 9

[Tony Mechelynck [:tonymec]]

Attachment: rearrange contents of newaddon.xul#restartPanel v2

I don't have the resources to compile my own Firefox, and I'm not sure how to request a try-build. Dave, could you please request one with this patch, on Win64 for arni2033, on Linux64 for me, and on your platform if you want one?

Comment 10

[Dave Townsend [:mossop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e902d07e999a

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to Tony Mechelynck [:tonymec] from comment #9)
> Created attachment 8720756 [details] [diff] [review]
> rearrange contents of newaddon.xul#restartPanel v2
> 
> I don't have the resources to compile my own Firefox

Just in case it's helpful to you - we now have "artifact builds" which downloads a pre-built Firefox automatically when you "./mach build" https://developer.mozilla.org/en-US/docs/Artifact_builds

This should be much faster and require far fewer resources to do a build, it's really handy if you're just doing frontend work like this.

Comment 12

[Tony Mechelynck [:tonymec]]

Attachment: screenshot in Russian with patch v2

The last paragraph is justified-right, which is not ideal, giving "Nightly." aligned-right with the last word of "Для завершения установки этого дополнения вы должны перезапустить"

New patch follows.

N.B. Rather than fiddle with the text I installed the latest 47.0a1 Russian langpack then restarted Firefox as "firefox --UILocale ru -P test-bug1247900" when authorizing the sideload: this gives Russian UI as "officially" translated.

Comment 13

[Tony Mechelynck [:tonymec]]

(In reply to Tony Mechelynck [:tonymec] from comment #12)
> Created attachment 8720992 [details]
> screenshot in Russian with patch v2
> 
> The last paragraph is justified-right, which is not ideal, giving "Nightly."
> aligned-right with the last word of "Для завершения установки этого
> дополнения вы должны перезапустить"

This seems to be intentional, however: see the following snippet of toolkit/themes/shared/extensions/newaddon.inc.css:

103 #restartMessage {
104   text-align: right;
105 }

So I'll make a patch removing this CSS rule (because IMHO the result is better without it)...

Comment 14

[Tony Mechelynck [:tonymec]]

Attachment: rearrange contents of newaddon.xul#restartPanel v2a

Please set r+ on EITHER patch v2 OR patch v2a BUT NOT BOTH.
Patch v2 has the restartMessage justified right as in attachment 8720992 [details].
Patch v2a aligns it normally (presumably left). No other change from v2.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8721007 [details] [diff] [review]
rearrange contents of newaddon.xul#restartPanel v2a

Review of attachment 8721007 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/newaddon.xul
@@ +46,5 @@
>  
>        <checkbox id="allow" label="&allow;"/>
>        <description id="later">&later;</description>
>  
> +      <deck id="buttonDeck" align=end>

You need quotes here or it isn't valid XML, I forgot to mention that I fixed that with the try push. But it also looks like we're doing this in the CSS elsewhere here so instead add -moz-box-align on the element in the CSS.

@@ +51,5 @@
>          <hbox id="continuePanel">
>            <button id="continue-button" label="&continue;"
>                    oncommand="continueClicked()"/>
>          </hbox>
> +        <vbox id="restartPanel">

This gains this css rule: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/extensions/newaddon.inc.css#93

Since it is now a vbox that centers the description and hbox below which end up being not what we want. You probably want align to be stretch and then on the hbox below add a -moz-box-pack: end to push the buttons all the way to the right.

@@ +56,1 @@
>            <spacer id="restartSpacer"/>

I don't think we need the spacer here anymore and you can remove it from the CSS too.

Comment 16

[Tony Mechelynck [:tonymec]]

(In reply to Dave Townsend [:mossop] from comment #15)
> Comment on attachment 8721007 [details] [diff] [review]
> rearrange contents of newaddon.xul#restartPanel v2a
> 
> Review of attachment 8721007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/content/newaddon.xul
> @@ +46,5 @@
> >  
> >        <checkbox id="allow" label="&allow;"/>
> >        <description id="later">&later;</description>
> >  
> > +      <deck id="buttonDeck" align=end>
> 
> You need quotes here or it isn't valid XML, I forgot to mention that I fixed
> that with the try push.

Ah, apparently I got misled by my HTML+CSS customs where words defined by the programmer (such as id=) are in quotes but keywords defined by the language aren't.

> But it also looks like we're doing this in the CSS
> elsewhere here so instead add -moz-box-align on the element in the CSS.

OK.
> 
> @@ +51,5 @@
> >          <hbox id="continuePanel">
> >            <button id="continue-button" label="&continue;"
> >                    oncommand="continueClicked()"/>
> >          </hbox>
> > +        <vbox id="restartPanel">
> 
> This gains this css rule:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/
> extensions/newaddon.inc.css#93
> 
> Since it is now a vbox that centers the description and hbox below which end
> up being not what we want. You probably want align to be stretch and then on
> the hbox below add a -moz-box-pack: end to push the buttons all the way to
> the right.

I could do that with a "child" rule but IMHO a new ID is more elegant, let's say #restartPanelButtons which ought to be unique.
> 
> @@ +56,1 @@
> >            <spacer id="restartSpacer"/>
> 
> I don't think we need the spacer here anymore and you can remove it from the
> CSS too.

OK.
New patch follows as soon as I've made the changes.

Comment 17

[Tony Mechelynck [:tonymec]]

Attachment: patch v3pre

The reason I'm requesting feedback rather than review is because of the (now commented-out) CSS rule at new line 103 in the CSS stylesheet: either (as I believe) that rule is indeed redundant with the -moz-box-pack part of the rule immediately above it, and then it must go completely; or else it isn't, and then it must stay without the /* */. In both cases this patch v3pre will be obsoleted by a v3.

Of course there may be other things I haven't thought of, of parts of your review above which I have misunderstood: I'll be happy if you point them out.

Comment 18

[Tony Mechelynck [:tonymec]]

Oh, and at new line 110 I wrote in "text-align: justify;" because I'm not sure of how otherwise to align left for LTR languages like English or French but right for RTL languages like Hebrew and Arabic.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8721606 [details] [diff] [review]
patch v3pre

Review of attachment 8721606 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem to align things correctly at all, I'm surprised it looks ok to you. To make it clear if it wasn't already we're trying to show the buttons in about the same space at the bottom right of the rest of the content (that will automatically move to the left in RTL languages). The current patch ends up centring the two buttons in the restart panel. I think my notes below will get what we want though.

::: toolkit/themes/shared/extensions/newaddon.inc.css
@@ +89,5 @@
>    margin-bottom: 20px;
>  }
>  
> +#buttonDeck {
> +  -moz-box-align: end;

This should be stretch for align and pack, this should make both the inner boxes expand to the full size of the deck.

@@ +97,5 @@
>  #restartPanel {
>    margin-top: 25px;
>    -moz-box-pack: end;
>    -moz-box-align: center;
>  }

continuePanel will need to use end for both pack and align to push its contents to the bottom left of the box.
restartPanel will need to use stretch for align and end for pack to make its contents fill the full width and drop to the bottom of the box.

@@ +108,3 @@
>  
>  #restartMessage {
> +  text-align: justify;

I think this is unnecessary, it should align appropriately according to the language.

Comment 20

[Tony Mechelynck [:tonymec]]

(In reply to Dave Townsend [:mossop] from comment #19)
> Comment on attachment 8721606 [details] [diff] [review]
> patch v3pre
> 
> Review of attachment 8721606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to align things correctly at all, I'm surprised it looks
> ok to you.

I still don't have the resources to compile my own, or the info (and possibly the privileges) about triggering try builds, so I have to go by intuition. Obviously I misunderstood some of your previous review. Thanks for making it clearer.

> To make it clear if it wasn't already we're trying to show the
> buttons in about the same space at the bottom right of the rest of the
> content (that will automatically move to the left in RTL languages). The
> current patch ends up centring the two buttons in the restart panel. I think
> my notes below will get what we want though.
> 
> ::: toolkit/themes/shared/extensions/newaddon.inc.css
> @@ +89,5 @@
> >    margin-bottom: 20px;
> >  }
> >  
> > +#buttonDeck {
> > +  -moz-box-align: end;
> 
> This should be stretch for align and pack, this should make both the inner
> boxes expand to the full size of the deck.
> 
> @@ +97,5 @@
> >  #restartPanel {
> >    margin-top: 25px;
> >    -moz-box-pack: end;
> >    -moz-box-align: center;
> >  }
> 
> continuePanel will need to use end for both pack and align to push its
> contents to the bottom left of the box.
> restartPanel will need to use stretch for align and end for pack to make its
> contents fill the full width and drop to the bottom of the box.
> 
> @@ +108,3 @@
> >  
> >  #restartMessage {
> > +  text-align: justify;
> 
> I think this is unnecessary, it should align appropriately according to the
> language.

New patch follows.

Comment 21

[Tony Mechelynck [:tonymec]]

Attachment: patch v3

I hope I understood this time.

Comment 22

[Tony Mechelynck [:tonymec]]

Attachment: patch v4

oops, "stretch for align and end for pack" <-- retartPanel

Comment 23

[Tony Mechelynck [:tonymec]]

P.S. https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

> Getting access to the Try Server
> 
> To use the try server, you'll need Level 1 Commit Access.
> 
> You can learn more about Mozilla's commit access policies and start the process
> of signing up for an account here: Becoming a Mozilla Contributor.

That's clear. I have no commit access of any kind, nor do I feel the need for one, so please request it for me on L64, for the reporter on W64, and for whatever you use if you want one.

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8722719 [details] [diff] [review]
patch v4

Unfortunately this still isn't right. Since you're not able to test this directly I think it's probably going to be quicker for me to just take over and do the final changes necessary to this patch rather than us going back and forth over bugzilla.

Comment 25

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1247900: Sideloaded add-ons page displays poorly when the restart message is too long for a single line.

Review commit: https://reviewboard.mozilla.org/r/36327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36327/

Comment 26

[Robert Helmer [:rhelmer]]

Comment on attachment 8723080 [details]
MozReview Request: Bug 1247900: Sideloaded add-ons page displays poorly when the restart message is too long for a single line.

https://reviewboard.mozilla.org/r/36327/#review33189

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a93848ae81dd

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a93848ae81dd

Comment 29

[arni2033]

Fixed on:   Win7_64, Nightly 47, 32bit, ID 20160229030448

All that page isn't minimalistic, but at least now all text lines look good. Comment 4 is also fixed