342221 - Microsummary picker has too much padding and doesn't block the popup when no microsummaries are available

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect)

Description

[Simon Bünzli]

Not sure whether the padding issue is due to Winstripe in particular or to XUL themeing in general. As for the popup issue, it might be worth considering replacing the menupicker with a textbox when no summaries are available.

Comment 1

[Simon Bünzli]

Attachment: screenshot (WinXP, after hitting [Down])

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Bug 341555, comment 4 suggests the following to fix the popup problem:

menulist#name[droppable="false"] > menupopup {display: none;}

The padding problem looks similar to bug 342182, which is for the same problem on the Mac.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Simon, does the patch (attachment 226588 [details] [diff] [review]) in bug 342182 solve this problem?

Comment 4

[Simon Bünzli]

Attachment: fix

No, attachment 226588 [details] [diff] [review] rather makes it worse (as you can see in Seth's screenshot).

Comment 5

[Myk Melez [:myk] [@mykmelez]]

> No, attachment 226588 [details] [diff] [review] [edit] rather makes it worse (as you can see in Seth's
> screenshot).

Based on Seth's description, it sounds like he got his screenshots backwards.  Or are you seeing the same effect with the patch in that bug applied?

Comment 6

[Simon Bünzli]

No, the screenshot is correct. Among other things, your patch incorrectly applies an additional white border around the widget's main border (as you can see on the right dialog in Seth's screenshot).

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Seth, can you try out the patch in this bug and see if it works better for you than my patch in bug 342182?

Comment 8

[(not reading, please use [e-mail] instead)]

> Seth, can you try out the patch in this bug and see if it works better for you
> than my patch in bug 342182?

yes.  i will do that and have an answer shortly.

Comment 9

[(not reading, please use [e-mail] instead)]

> yes.  i will do that and have an answer shortly.

this patch fixes the problem.  see the upcoming screen shot.

additionally, when there is no summary, the down key does not show me a menupicker (good catch!).

screen shot (with patch applied on win32 1.8 branch) coming...

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: screen shot (after applying the patch in this bug)

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 226631 [details] [diff] [review]
fix

With this patch applied, a Linux build continues to look decent (not perfect, per bug 342392, but we can deal with that separately).

> menulist#name {
...
>+menulist#name > .menulist-editable-box {

These should both be menulist#name[droppable="false"], since we'll want to use the default style for editable menulists if microsummaries are available in the drop-down menu.  If there are problems with that style, we should fix them in the stylesheet for all editable menulists.


>-  /* These rules come from the textbox element in textbox.css. */

Nit; it'd be good to leave this comment in so folks know where to look if this regresses in the future because the style of textboxes changes.


>+/* Adjust the menulist for when there's not drop-down menu */
>+menulist#name[droppable="false"] {
>+  cursor: text;
>+}

This can be moved to the section above once that section's selector is also menulist#name[droppable="false"].

Otherwise this patch looks good.  I'll remove my win32 changes from bug 342182 in favor of these.

Comment 12

[Simon Bünzli]

Attachment: fix (nits addressed)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 226684 [details] [diff] [review]
fix (nits addressed)

Code looks good, works as advertised. r=myk

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Patch checked in to trunk.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 226684 [details] [diff] [review]
fix (nits addressed)

Notes for drivers considering the approval1.8.1 request:

This patch has just landed on the trunk, but letting it bake for a couple days won't help test it, because it's specific to the old bookmarks code, which is disabled on the trunk.

This patch fixes a recent regression in bookmarks dialog appearance and behavior.  There is little risk to the patch (it's entirely CSS), although it's possible (but unlikely) for the patch to introduce a new regression in the process of fixing the current one.

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 226684 [details] [diff] [review]
fix (nits addressed)

a=mconnor for branch drivers