611525 - Create HUD style for name field in edit bookmark panel when it is a droppable menulist

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[:Margaret Leibovic]

Attachment: patch

We're currently using the native styles for the name field when it is a droppable menulist, which looks bad. Here's a patch that fixes it.

(This patch is built on top of the patch for bug 597557)

Comment 1

[:Margaret Leibovic]

Attachment: before and after screenshots

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 489989 [details] [diff] [review]
patch

Hm, that's a bunch of code that would hardly ever be used (since microsummaries are more or less dead (bug 611676)), which makes this both regression-prone and probably not worth taking...

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Hm, that's a bunch of code that would hardly ever be used (since microsummaries
> are more or less dead (bug 611676))

Err, bug 524091.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

while I suppose microsummaries generators will be dropped pretty soon, I also guess support for existing microsummaries on pages will survive with some resize (thus their visualization). Also bugzilla has microsummaries.

I don't have strong feelings on the style, so I won't complain in both cases, but the screenshot is cool and gives much less sense of unpolished.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> while I suppose microsummaries generators will be dropped pretty soon, I also
> guess support for existing microsummaries on pages will survive with some
> resize (thus their visualization). Also bugzilla has microsummaries.

This belongs in bug 524091, but I think we should drop the whole thing if basically nobody wants to use it. I don't think Mozilla projects count...

Comment 6

[:Margaret Leibovic]

I don't care too much either way. I was just thinking that we should make the feature look correct, as long as it's included in the product.

Comment 7

[Dão Gottwald [:dao]]

"unpolished", "look correct" etc. only apply if someone actually sees it... which is mostly not the case here, as far as I can tell. Even if we keep the feature for the time being (mostly for the lack of somebody committing to removing it, I guess), we shouldn't be taking arbitrary code to polish or improve it. This would be different if the patch was just setting a missing CSS class or something. As it is, I don't think it pays off.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Your "nobody sees it" point, hurts against the fact this is in our primary ui for bookmarking. Just bookmark a bugzilla search and you'll see it.

And we are not keeping microsummaries just because nobody has time to remove them, but because that requires a discussion regarding the direction we want to take. I think in the end we will kill most of them, but not their visualization thus this bug is valid.

The patch is pretty much contained and I don't see the regression-prone stuff here since these styles apply to just 1 element, I'd agree if it would apply to the full panel or multiple elements.
Thus I don't see a reason to drop this work. I'll ask a ui-r to Mike to get a decision.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 489990 [details]
before and after screenshots

Mike, could you please give us an evaluation if you think it is worth to take this change for style coherence in the panel?

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Your "nobody sees it" point, hurts against the fact this is in our primary ui
> for bookmarking. Just bookmark a bugzilla search and you'll see it.

For hundreds of millions pages out there, it's not in our primary UI.

> And we are not keeping microsummaries just because nobody has time to remove
> them, but because that requires a discussion regarding the direction we want to
> take.

Ok. I'm not prepared to take this patch until that discussion has been had.

> The patch is pretty much contained and I don't see the regression-prone stuff
> here since these styles apply to just 1 element, I'd agree if it would apply to
> the full panel or multiple elements.

I didn't mean that the patch itself could cause regressions. I meant that it's unlikely that somebody would notice it if the styling broke, e.g. when somebody touches the panel style again.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Fwiw, not implementing fixes because somebody in future could break them, is a weak argument.
FX4 will have microsummaries, FX5 could be different, but we should base our current decisions on the product we are releasing imo.
I'm fine if we don't want to fix this style, but I'd like an acceptable argument to do so.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> I'm fine if we don't want to fix this style, but I'd like an acceptable
> argument to do so.

The onus lies on the code wanting to get in, it needs to prove worthy. The fact that we still ship the feature isn't necessarily sufficient, e.g. if the feature isn't used despite being shipped.

Let me use this opportunity to mention that this bug goes back to Firefox 3.0, I think, but nobody seems to have cared... except for Margaret when she worked on the code!

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The fact this is not a regression is a good enough argument to me, thanks. I think this now has enough info for a driver to take a decision.

Comment 14

[Mil]

Dão, you really **** me off.

Comment 15

[Dão Gottwald [:dao]]

My pleasure!

Comment 16

[:Margaret Leibovic]

We won't fix this because Marco is fixing bug 524091.