780442 - [ARMv6] About:Memory page buttons are overlapping, oversized

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Kami]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:16.0) Gecko/16.0 Firefox/16.0
Build ID: 20120802073706

Steps to reproduce:

enter to:
about:config or
about:memory pages, and check the copy to clipboard, GC, etc butons


Actual results:

Buttons are overlapping, wrong sized, with oversized text

Comment 1

[Mark Capella [:capella]]

In the original description do you mean the "about:support" and "about:memory" pages?

about:support has a |Copy to Clipboard| button problem I'm addressing in bug 774500 ...

about:memory has the |Update|, |GC|, |CC|, and |Minimize memory usage buttons|, and they are indeed goofy looking. ( There seems to be a problem rendering buttons ... I thought maybe: Bug 783492 )

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

This patch is for the about:memory screen and is similar to the about:support patch ... it enlarges the data text and polishes up the four buttons @ the bottom of the screen.

(Update, GC, CC, Minimize memory usage)

Comment 3

[:Margaret Leibovic]

Comment on attachment 655050 [details] [diff] [review]
Patch (v1)

Nice job picking up this bug. Another suggestion I have would be to add some padding and margins to the buttons, since they're still relatively small, so it could be easy to accidentally hit the wrong one.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

Try this version ... nicer !

One last question though, on the bottom of the page there's a literal string:
"Hover the pointer over the name of a memory report to see a description of what it measures."

This doesn't work right/ / should be made "hidden" for mobile?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Capella [:capella] from comment #4)

> One last question though, on the bottom of the page there's a literal string:
> "Hover the pointer over the name of a memory report to see a description of
> what it measures."
> 
> This doesn't work right/ / should be made "hidden" for mobile?

If we are not overriding the DTD, then I think hiding it sounds good. Hover won't be easy to implement anyway.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok ... asking for review then ...

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v4)

Fixed my own nits ...

Comment 8

[:Margaret Leibovic]

Comment on attachment 655474 [details] [diff] [review]
Patch (v4)

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

r- for creating a new class name, but I'd also like to see the style declarations cleaned up. However, I tested this on my phone, and it looks really good! Thanks!

::: mobile/android/themes/core/aboutMemory.css
@@ +90,5 @@
> +  margin-left: 1%;
> +  width: 30%;
> +  margin-right: 2%;
> +  padding-bottom: 2%;
> +  margin-bottom: 1%;

I think it would be clearer if we try to minimize the separate margin/padding declarations. For example, the #updateButton margin/padding styles could be reduced to:

margin: 1% 2% 1% 1%;
padding: 2% 0;

I had a hard time reading through these declarations, so I think that would help anyone stumbling in here in the future :)

::: mobile/android/themes/core/jar.mn
@@ +7,5 @@
>  chrome.jar:
>  % skin browser classic/1.0 %skin/
>    skin/aboutPage.css                        (aboutPage.css)
>    skin/about.css                            (about.css)
> +  skin/aboutMemory.css                      (aboutMemory.css)

Nit: Put this in correct alphabetical order with the other about pages.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +449,5 @@
>    let legendText2 = "Hover the pointer over the name of a memory report " +
>                      "to see a description of what it measures.";
>  
>    appendElementWithText(body, "div", "legend", legendText1);
> +  appendElementWithText(body, "div", "legend2", legendText2);

I don't like making a new class name like this just for being able to hide that element. Instead, you could just use .legend:last-of-type as the selector in the mobile CSS to hide this element.

Comment 9

[Mark Capella [:capella]]

Attachment: Patch (v5)

Thanks! (Nice CSS style-tweaks). Here's the updated version.

Comment 10

[:Margaret Leibovic]

Comment on attachment 656954 [details] [diff] [review]
Patch (v5)

This looks good, thanks.

However, did you also get a chance to play around with adding a <meta name="viewport"> tag to this page like you did in bug 733169? I wonder if that would help with the font sizes, so that you wouldn't need to manually make them larger. Feel free to land this patch as-is, but if you find that makes things better, I can review a new version.

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v6)

No, I hadn't gone further before, thinking it was ok, since that portion was working as-is and the CSS needed to be modified anyhow for the button corrections.

But the attached tweaks it to use the META tag, and removes the font-size XX-LARGE stuff. This actually takes a teeny-little more screen (width) space, but still works nicely :)

Comment 12

[:Margaret Leibovic]

Comment on attachment 656997 [details] [diff] [review]
Patch (v6)

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

Just one nit, but looks good.

::: mobile/android/themes/core/aboutMemory.css
@@ +35,5 @@
> +.mrPerc {
> +}
> +
> +.mrSep {
> +}

You should get rid of these unused selectors.

Comment 13

[Mark Capella [:capella]]

push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=00c8a878e5c5

Comment 14

[Mark Capella [:capella]]

On to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3e83ba39ec95

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52abdd1fbcc8

Comment 16

[Nicholas Nethercote [inactive]]

As the author of about:memory, I was surprised to find that it had changed and I was quite unaware of it.  This change was small but I would nonetheless appreciate being CC'd and probably r?'d on such changes in the future.  The appropriate Bugzilla product/component is "Toolkit"/"about:memory".


> On to inbound:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3e83ba39ec95

You're supposed to link to the changesets on hg.mozilla.org, not to the push on tbpl.mozilla.org.

Comment 17

[Nicholas Nethercote [inactive]]

It's also interesting that four vanilla buttons show up strangely.  Is there an underlying problem that needs to be fixed, rather than tweaking the presentation of the buttons?

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> It's also interesting that four vanilla buttons show up strangely.  Is there
> an underlying problem that needs to be fixed, rather than tweaking the
> presentation of the buttons?

Especially now that I see (urk) all four buttons have *different* styles.  I'm about to add a new button for bug 768470 and I don't particularly want to muck about with ad hoc styling to get it to work on Android -- that's absurd.

Comment 19

[Nicholas Nethercote [inactive]]

After discussion with Jesse, Unfocused and dbaron on IRC, I have backed this out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a75329c54a3

I would have given r- if I'd been asked, because this is an ad hoc fix that is unlikely to work if the device, font, or button text changes.  I also don't want to be on the hook for "breaking" it when I add a new button in bug 768470.  If there is an underlying bug with <button> rendering on mobile, please file a bug for that.

Comment 20

[Mark Capella [:capella]]

Ouch! The original problem was buttons not rendering properly in Android ... Override the CSS allowed me to fix them without affecting the base Firefox code ... (sorry I know you can read the above)

Comment 21

[:Margaret Leibovic]

njn, sorry for not cc'ing you on this bug, and you're right that this belongs in the about:memory component. I just assumed that no one thought about making this look good on Android, and in the past we've overridden CSS files to make changes like this to minimize the impact on desktop, so that's why I was okay with this approach. However, if we only want to maintain one style sheet, we can use responsive design practices to make sure it works on different screen sizes. I think we (the mobile team) have grown accustomed to needing to modify things for Android on our own, but this could be a case where it's good for the main developers working on a feature to be responsible for making sure things work on Android as well, especially if you're adding new things, like a new button.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

I'm fine with overriding the CSS if needed, but we shouldn't tweak each button individually. That makes it hard to add new buttons.

Comment 23

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/3a75329c54a3

Comment 24

[Mark Capella [:capella]]

(Lost track of this...)

So the existing problem as appearing in Android, is corrected more appropriately in Toolkit, they will fix it, and I'm effectively excused from this?

Comment 25

[Nicholas Nethercote [inactive]]

It does sound like there's a problem in the underlying code for button elements.  As for whether you're "excused" from this bug, I guess that's your decision.  How important is this?  Do you have other things more important to work on?  Etc.

Comment 26

[Mark Capella [:capella]]

Attachment: about:memory screenshot

Updated repo and rebuilt ... fyi how the current nightly page displays on my device, Galaxy SIII ...

Comment 27

[Mark Capella [:capella]]

Attachment: Patch (v7)

Would this patch be better / less intrusive to existing code?

Comment 28

[Mark Capella [:capella]]

Attachment: about:memory w/new change

Comment 29

[Nicholas Nethercote [inactive]]

Comment on attachment 660364 [details] [diff] [review]
Patch (v7)

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

Much better!  Thanks for doing this.  It still feels like a bug that the button padding is needed at all, but this will do.

::: mobile/android/themes/core/aboutMemory.css
@@ +17,5 @@
> +h2 {
> +  background: #ddd;
> +  padding-left: .1em;
> +}
> +

You've omitted a few rules from the original aboutMemory.css:  .badInputWarning, .mrPerc, .mrSep.  Please put them back.

@@ +21,5 @@
> +
> +button {
> +  margin: 1%;
> +  padding: 2%;
> +}

Can you add a comment on this, something like "buttons are different sizes and overlapping without this".

@@ +50,5 @@
> +}
> +
> +.hasKids:hover {
> +  text-decoration: underline;
> +}

Does :hover work on mobile?  You could probably remove this rule.

@@ +64,5 @@
> +}
> +
> +.legend:last-of-type {
> +  display: none;
> +}

Is this hiding the "Hover the pointer..." line at the bottom?  That's non-obvious and if we add another line at the bottom the wrong one will be hidden.  I suggest you add a new class called "hiddenOnMobile" and make that "Hover the pointer..." element have the "legend" and the "hiddenOnMobile" classes.

::: toolkit/components/aboutmemory/content/aboutMemory.xhtml
@@ +8,5 @@
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
>      <!-- the <title> is filled in by aboutMemory.js -->
> +    <meta name="viewport" content="width=device-width; user-scalable=false"/>

Does this change how about:memory looks on desktop?

Comment 30

[Mark Capella [:capella]]

B) Cool ! Thanks !

fyi,

We added padding to make the buttons larger / easier to select when grouped together on a small display device.

.badInputWarning appears new, I'll be happy to add to the android CSS version.

.mrPerc and .mrSep were empty definitions, and I was asked to remove them in android version - comment 12 ...

button style comment will be added...

Yah, I overlooked the .hasKids:hover style definition ... agrees it should come out. Also, I'll re-do the approach to hiding the text with the extra element class versus our last-of-type approach.

No, the meta tag doesn't affect desktop rendering. We used a similar approach in bug 733169 to fix about:buildconfig textsize for android without affecting desktop.

I'll make / test the changes and re-post, and of course re-submit for final review to both margaret, and you.

Comment 31

[Mark Capella [:capella]]

Attachment: Patch (v8)

Small changes ... display remains the same as the second screenshot I posted ...

Comment 32

[Nicholas Nethercote [inactive]]

Comment on attachment 660391 [details] [diff] [review]
Patch (v8)

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

Looks good, thanks!

I myself would keep the empty .mrPerc and .mrSep rules to make it consistent with the existing aboutMemory.css, but I won't insist on it.

It might be useful to have a comment at the top of the original aboutMemory.css saying that the version used for mobile is at mobile/android/themes/core/aboutMemory.css?  I would find that a useful reminder.

Comment 33

[:Margaret Leibovic]

Comment on attachment 660391 [details] [diff] [review]
Patch (v8)

I agree with what njn's comments. r+ to the /mobile changes.

Comment 34

[Mark Capella [:capella]]

Attachment: Patch (v9) Final Version

Attaching the final version after updating to include njn's final comments.

Tested on my device, and local WIN desktop, now sending to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=6ac4cafcebd7

Comment 35

[Mark Capella [:capella]]

Looks good, on to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1b8e89614a6c

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Capella [:capella] from comment #35)
> Looks good, on to inbound:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1b8e89614a6c

http://hg.mozilla.org/integration/mozilla-inbound/rev/1b8e89614a6c

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b8e89614a6c

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b8e89614a6c

Comment 39

[Mark Capella [:capella]]

This was marked as [leave open] while we decided how best to fix it. Since we've now completed this, I'd like to go ahead and mark it Resolved / Fixed.

njn ... good with you?

Comment 40

[Nicholas Nethercote [inactive]]

Now that it's been merged to mozilla-central, we can close it.  Thanks!