Finish new theme styles for dialogs/prompts

VERIFIED FIXED

Status

defect
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: mbrubeck, Assigned: wesj)

Tracking

({polish})

Dependency tree / graph

Firefox Tracking Flags

(fennec2.0next+)

Details

Attachments

(3 attachments, 10 obsolete attachments)

Fennec's dialogs should have dark headers with light text, and larger header fonts (similar to native Android dialogs).

This was intended to be part of the new Fennec theme (bug 575403) but fell through the cracks in the rush to ship beta 2.
tracking-fennec: --- → ?
We really want some change here, but it's not really blocking. Let's get a design.
tracking-fennec: ? → 2.0-
Posted patch WIP (obsolete) — Splinter Review
Talked to finkle about this a bit the other day, and he said he might take something. This may go further than just that, but looks good.

Flickr set:
http://www.flickr.com/photos/digdug2k/sets/72157626190588906/with/5495045095/
Here's a separate patch to add arrows to the "New Tab" popup (madhava asked). Pics are in the flickr stream.
Attachment #516744 - Attachment is patch: true
Attachment #516744 - Attachment mime type: application/octet-stream → text/plain
Assignee: mbrubeck → wjohnston
Posted patch Arrowbox changes (obsolete) — Splinter Review
Arrowboxes changes in their own patch. Putting this separate piece up after talking to madhava about this on irc:

(02:52:32 PM) madhava: the popups I'd take (if it were up to me)
(02:53:01 PM) madhava: dialogs, because the content is so varied leading to potential layout weirdness, I might wait on
(02:53:17 PM) wesj: madhava: ahh, you mean just the arrowbox ones, eh?
(02:53:25 PM) madhava: yeah
(02:53:49 PM) madhava: just thinking about benefits vs. potential issues
(02:55:01 PM) wesj: madhava: hmm... ok. i'll try to separate that piece out and put it up separately

Putting up for review as potential low-risk polish.
Attachment #516744 - Attachment is obsolete: true
Attachment #517566 - Flags: review?(mark.finkle)
can you post before/after screenshots?
Sure. They are in the flickr stream posted about, but here is the bookmarks one:

http://dl.dropbox.com/u/72157/Dialogs/Bookmark.png

and the new tab ones on two different backgrounds:

http://dl.dropbox.com/u/72157/Dialogs/arrow2.png
http://dl.dropbox.com/u/72157/Dialogs/ArrowOpen.png
I definitely like the way these look. What's the patch status?
(In reply to comment #6)
> and the new tab ones on two different backgrounds:
> 
> http://dl.dropbox.com/u/72157/Dialogs/arrow2.png
> http://dl.dropbox.com/u/72157/Dialogs/ArrowOpen.png

Just in case, do we want the arrow when the new tab panel is over the awesome panel since the user can't pan to the right?
(In reply to comment #8)
> (In reply to comment #6)
> > and the new tab ones on two different backgrounds:
> > 
> > http://dl.dropbox.com/u/72157/Dialogs/arrow2.png
> > http://dl.dropbox.com/u/72157/Dialogs/ArrowOpen.png
> 
> Just in case, do we want the arrow when the new tab panel is over the awesome
> panel since the user can't pan to the right?

Too complicated. Either we keep the arrows or we ditch them - all the time.
Close. A little cleanup, some problems with textboxes in the sync-setup stuff,
and (scarier) maybe making the width more dynamic (I think in the current sytle
it is currently fixed, but our margins are bigger so it doesn't look quite so
awkward).

Arrow box patch is ready for review. I don't think the newtab popup is
currently even shown if the sidebar is open. There's a bug because I
accidentally yanked the Browser.pushPopup stuff that causes it not to hide if
you pan the sidebar open while the popup is showing. Updated patch coming...

I'll start the last clean up on the other guy too
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > and the new tab ones on two different backgrounds:
> > > 
> > > http://dl.dropbox.com/u/72157/Dialogs/arrow2.png
> > > http://dl.dropbox.com/u/72157/Dialogs/ArrowOpen.png
> > 
> > Just in case, do we want the arrow when the new tab panel is over the awesome
> > panel since the user can't pan to the right?
> 
> Too complicated. Either we keep the arrows or we ditch them - all the time.

and finishing my thought... we mainly use the arrow to let the user know where
something new was added, not as an indication to pan. So I think leaving the
arrows on the box is fine for now.

Personally, I'd vote to remove the arrows altogether, but that's a different
bug.
Attachment #517566 - Attachment is obsolete: true
Attachment #517921 - Flags: review?(mark.finkle)
Attachment #517566 - Flags: review?(mark.finkle)
Comment on attachment 517921 [details] [diff] [review]
Arrowbox patch v2

Looks good. Not sure if we'll get this in 4.0 or not yet.
Attachment #517921 - Flags: review?(mark.finkle) → review+
Re-noming. I have a case for making the UI look like a conhesive unit and not a mis-mash of different styles
tracking-fennec: 2.0- → ?
Posted patch Dialogs (obsolete) — Splinter Review
OK, this seems to work well. Need to test a few more things, including nsIPromptService.select dialogs. I'm going to write some test stuff and pound away for today, but wanted to get review underway at the same time.

I'm a bit nervous about how this will look on tablets.
Attachment #516712 - Attachment is obsolete: true
Attachment #518144 - Flags: review?(mbrubeck)
Attachment #518144 - Flags: review?(mark.finkle)
Patch applies on top of the arrowbox one
Attachment #517921 - Flags: approval2.0+
Comment on attachment 518144 [details] [diff] [review]
Dialogs

Probably harmless, but I do see a number of class="prompt-button" still around, even though this class is no longer used by any stylesheet - one in components/PromptService.js and several in chrome/content/prompt/*.xul.

>+.prompt-checkbox-label {
>+  text-align: left;
> }

>+.prompt-edit {
>+  margin: @margin_xnormal@;
>+  font-size: @font_normal@;
>+  text-align: left;
>+}

Should probably both be "text-align: start;"
Attachment #518144 - Flags: review?(mbrubeck) → review+
Whiteboard: [has patch]
Posted patch Patch v1.1 (obsolete) — Splinter Review
I played with this on a Xoom, but even on Desktop if you open a prompt and make Fennec full screen, we stretch the dialog to cover the screen width. The old prompts also did this.

This just adds a max-width to the dialog. Also adds the text-align: start mbrubeck caught. I will remove the "prompt-button" later.
Attachment #518144 - Attachment is obsolete: true
Attachment #518224 - Flags: review?(mark.finkle)
Attachment #518144 - Flags: review?(mark.finkle)
Posted patch Patch v1.1 (obsolete) — Splinter Review
Argh. Forgot to qref, but this is causing a problem with they sync dialog anyway...
Attachment #518224 - Attachment is obsolete: true
Attachment #518226 - Flags: review?(mark.finkle)
Attachment #518224 - Flags: review?(mark.finkle)
Posted patch Patch v1.2 (obsolete) — Splinter Review
Switched syncsetup to use the same modal-block class as normal dialogs. Sync dialogs scale better on large screens with this too. Updated the build to reflect the fix. Same url:

http://dl.dropbox.com/u/72157/fennec.apk
Attachment #518226 - Attachment is obsolete: true
Attachment #518231 - Flags: review?(mark.finkle)
Attachment #518226 - Flags: review?(mark.finkle)
Attachment #518231 - Attachment is patch: true
Attachment #518231 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 518231 [details] [diff] [review]
Patch v1.2

This looks good, but I think there is a bug in the new way the Sync dialog is handled. Try opening the dialog, then "cancel" it. I am now stuck in the preferences panel - back button (on Android) doesn't get me out. Seems like a problem with how we pop the dialog?
Attachment #518231 - Flags: review?(mark.finkle) → review-
BTW, The dialog patch won't nake it into Fennec 4, but we will try to land it ASAP after branching.
tracking-fennec: ? → 2.0next+
Posted patch Patch v2 (obsolete) — Splinter Review
I had used our modal-block class on the syncsetup dialog. Didn't realize that in some places we actually check for elements with className="modal-block" to determine if a dialog is already open. Changed it to "perm-modal-block" here, and I actually just removed those elements in 641639.

This also updates the edit bookmark dialog to use the same classes for styling.
Attachment #518231 - Attachment is obsolete: true
Attachment #519456 - Flags: review?(mark.finkle)
Duplicate of this bug: 606581
Posted patch Test fixes (obsolete) — Splinter Review
Whoops. This also breaks a few tests I wrote that
1.) use getElementsByClassName to get prompt buttons. since the class is shared with the syncbuttons we pick up both

2.) assume the title of dialogs is a label, not a description.

I also removed a bunch of logging that I accidentally slipped into the test stuff.
Attachment #520357 - Flags: review?(mark.finkle)
Comment on attachment 519456 [details] [diff] [review]
Patch v2

Let's get an updated patch, in case it's bit rotted and see if we can get this landed for Firefox 5.
Attachment #519456 - Flags: review?(mark.finkle) → review+
Comment on attachment 520357 [details] [diff] [review]
Test fixes

Make sure these still pass on linux desktop
Attachment #520357 - Flags: review?(mark.finkle) → review+
Posted patch Patch v2.1 (obsolete) — Splinter Review
Unbitrotted patch
Attachment #519456 - Attachment is obsolete: true
Attachment #524683 - Flags: review+
Unbitrotted tests for checkin.
Attachment #520357 - Attachment is obsolete: true
Here's a build with this patch for testing:

http://dl.dropbox.com/u/72157/fennec.apk
(In reply to comment #32)
> Here's a build with this patch for testing:
> 
> http://dl.dropbox.com/u/72157/fennec.apk

The build looks and works OK except for one issue:

1. Go to Preferences
2. Tap "Back" to leave Preferences
3. I am trapped!

At first I thought it was because I opened the Sync "Connect" dialog or the Clear Private Data "Clear" prompt, but it happens all the time.

We need to find out if it's a problem with the patch or something else.

Also, be sure to test the Sync and Clear Private Data prompts too. Make sure we can go "Back" from Preferences
Just realized I moved this to my dropbox folder but wasnt running dropboz. ill update the build in a bit.
The link should be correct now. Soryy :(
Attachment #524683 - Attachment is obsolete: true
Attachment #525211 - Flags: review+
Attachment #524691 - Flags: review+
Whiteboard: [has patch]
VERIFIED FIXED on: 

Build Id: Mozilla /5.0 (Android;Linux armv7l;rv:2.2a1pre) Gecko/20110412 Firefox/4.2a1pre Fennec /4.1a1pre 

Device: HTC Desire Z (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.