Closed Bug 780442 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: about:memory, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: kami911, Assigned: capella)

References

Details

(Whiteboard: [ARMv6])

Attachments

(4 files, 7 obsolete files)

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
Blocks: ARMv6-QA
Whiteboard: [ARMv6]
No longer blocks: ARMv6-QA
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 )
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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)
Attachment #655050 - Flags: feedback?(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.
Attachment #655050 - Flags: feedback?(margaret.leibovic) → feedback+
Summary: [ARMv6] about:config, about:memory pages' buttons are overlapping, oversized on Galaxy XCover → [ARMv6] About:Memory page buttons are overlapping, oversized
Attached patch Patch (v2) (obsolete) — Splinter Review
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?
Attachment #655050 - Attachment is obsolete: true
(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.
Attached patch Patch (v3) (obsolete) — Splinter Review
Ok ... asking for review then ...
Attachment #655459 - Attachment is obsolete: true
Attachment #655469 - Flags: review?(margaret.leibovic)
Attached patch Patch (v4) (obsolete) — Splinter Review
Fixed my own nits ...
Attachment #655469 - Attachment is obsolete: true
Attachment #655469 - Flags: review?(margaret.leibovic)
Attachment #655474 - Flags: review?(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.
Attachment #655474 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch (v5) (obsolete) — Splinter Review
Thanks! (Nice CSS style-tweaks). Here's the updated version.
Attachment #655474 - Attachment is obsolete: true
Attachment #656954 - Flags: review?(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.
Attachment #656954 - Flags: review?(margaret.leibovic) → review+
Attached patch Patch (v6) (obsolete) — Splinter Review
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 :)
Attachment #656954 - Attachment is obsolete: true
Attachment #656997 - Flags: review?(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.
Attachment #656997 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/52abdd1fbcc8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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.
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?
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [ARMv6] → [ARMv6][leave open]
Attachment #656997 - Flags: review-
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)
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.
Assignee: markcapella → nobody
Component: General → about:memory
Product: Firefox for Android → Toolkit
Target Milestone: Firefox 18 → ---
Version: Firefox 17 → Trunk
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.
(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?
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.
Attached image about:memory screenshot
Updated repo and rebuilt ... fyi how the current nightly page displays on my device, Galaxy SIII ...
Attached patch Patch (v7) (obsolete) — Splinter Review
Would this patch be better / less intrusive to existing code?
Attachment #656997 - Attachment is obsolete: true
Attachment #660364 - Flags: feedback?(n.nethercote)
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?
Attachment #660364 - Flags: feedback?(n.nethercote) → feedback+
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.
Attached patch Patch (v8)Splinter Review
Small changes ... display remains the same as the second screenshot I posted ...
Attachment #660364 - Attachment is obsolete: true
Attachment #660391 - Flags: review?(n.nethercote)
Attachment #660391 - Flags: review?(margaret.leibovic)
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.
Attachment #660391 - Flags: review?(n.nethercote) → review+
Comment on attachment 660391 [details] [diff] [review]
Patch (v8)

I agree with what njn's comments. r+ to the /mobile changes.
Attachment #660391 - Flags: review?(margaret.leibovic) → review+
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
Assignee: nobody → markcapella
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?
Now that it's been merged to mozilla-central, we can close it.  Thanks!
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [ARMv6][leave open] → [ARMv6]
You need to log in before you can comment on or make changes to this bug.