618989 - Finish new theme styles for dialogs/prompts

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

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.

Comment 1

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

We really want some change here, but it's not really blocking. Let's get a design.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: WIP

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/

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Separate patch to style newTab

Here's a separate patch to add arrows to the "New Tab" popup (madhava asked). Pics are in the flickr stream.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Arrowbox changes

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.

Comment 5

[Stuart Parmenter]

can you post before/after screenshots?

Comment 6

[Wesley Johnston (:wesj)]

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

Comment 7

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

I definitely like the way these look. What's the patch status?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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?

Comment 9

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

(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.

Comment 10

[Wesley Johnston (:wesj)]

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

Comment 11

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

(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.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Arrowbox patch v2

Comment 13

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

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.

Comment 14

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

Re-noming. I have a case for making the UI look like a conhesive unit and not a mis-mash of different styles

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Dialogs

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.

Comment 16

[Wesley Johnston (:wesj)]

Patch applies on top of the arrowbox one

Comment 17

[Wesley Johnston (:wesj)]

Build for testing:

http://dl.dropbox.com/u/72157/fennec.apk

Comment 18

[Wesley Johnston (:wesj)]

Pushed arrowbox part:
http://hg.mozilla.org/mobile-browser/rev/2dc7f7b3c40d

Comment 19

[Matt Brubeck (:mbrubeck)]

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;"

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

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.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

Argh. Forgot to qref, but this is causing a problem with they sync dialog anyway...

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v1.2

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

Comment 23

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

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?

Comment 24

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

BTW, The dialog patch won't nake it into Fennec 4, but we will try to land it ASAP after branching.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Test fixes

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.

Comment 28

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

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.

Comment 29

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

Comment on attachment 520357 [details] [diff] [review]
Test fixes

Make sure these still pass on linux desktop

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Unbitrotted patch

Comment 31

[Wesley Johnston (:wesj)]

Attachment: Test fixes for checkin

Unbitrotted tests for checkin.

Comment 32

[Wesley Johnston (:wesj)]

Here's a build with this patch for testing:

http://dl.dropbox.com/u/72157/fennec.apk

Comment 33

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

(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

Comment 34

[Wesley Johnston (:wesj)]

Just realized I moved this to my dropbox folder but wasnt running dropboz. ill update the build in a bit.

Comment 35

[Wesley Johnston (:wesj)]

The link should be correct now. Soryy :(

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Patch for checkin

Comment 37

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/27718fc39386
http://hg.mozilla.org/mozilla-central/rev/8b0b0bc824c0

Comment 38

[Anna (Waverley)]

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)