479957 - content is rendered outside the window if width is narrower than findbar if OS's theme is Classic

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Atsushi Sakai]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090218 Minefield/3.2a1pre

Steps to reproduce:
1. Make the OS's theme Classic.
2. Open the findbar.
3. Make the browser window narrower than the findbar.

Actual result:
The content is rendered outside the window, the same symptom as Bug 367622.
When "Reached end of page,..." is displayed, the width of content gets wider, then highlighted text moves.
This makes findbar unusable under certain situation.

Expected result:
The content should be rendered inside the window.


The root cause may be fixed by Bug 428939 or Bug 458231
but some workaround is needed for now.

Comment 1

[Atsushi Sakai]

Attachment: Patch v1.0 for branch

"Work around bug 458231" in Bug 69710 triggers this issue.
This is the workaround for the workaround.

Comment 2

[Damon Sicore (:damons)]

Would take a patch, but don't think this issue would hold 1.9.1.  Please re-nom if you disagree.

Comment 3

[Atsushi Sakai]

re-nom blocking.
This is a regression (Fx3.0.x works fine), and it degrades Find Toolbar, which is a major function in Firefox.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

This appears to be a duplicate of bug 428939.

Comment 5

[Atsushi Sakai]

(In reply to comment #4)
> This appears to be a duplicate of bug 428939.

The root cause is the same (I mentioned in Comment #0), but findbar issue especially degrade user experience because the findbar is changing its width depending on find status and become much bigger than 400px (for example, 960px with Japanese Firefox on Japanese Windows).
So I think findbar issue should block 1.9.1.

If bug 428939 can't get fixed on 1.9.1, findbar needs some workaround.
I meant this bug is for the workaround.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

I agree with Damon that I don't think this blocks, but we'd take the patch. I'll ask Gavin to do a quick review and would be happy to approve it.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 363867 [details] [diff] [review]
Patch v1.0 for branch

>diff -r 69c86f3acc5a toolkit/themes/winstripe/global/findBar.css

>+checkbox[disabled="true"]:-moz-system-metric(windows-classic) {
>+  color: GrayText;
>+  text-shadow: none;
>+}

Why is this rule needed? The other two negate the changes in attachment 347530 [details] [diff] [review], but I don't see how this one is relevant.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> (In reply to comment #4)
> > This appears to be a duplicate of bug 428939.
> 
> The root cause is the same (I mentioned in Comment #0)

Well, does this patch fix all symptoms described in that bug? The core issue really bug 204743, I suppose, but until that bug is fixed, changes in theme CSS are really fixes rather than "workarounds".

Comment 9

[Atsushi Sakai]

(In reply to comment #8)
> Well, does this patch fix all symptoms described in that bug?

This patch fixes only the issue with Find Toolbar, which is not mentioned in bug 428939.
In comparison with findbar, I think the issues mentioned in bug 428939 are not important because normal toolbars can be narrower than findbar, and also doesn't change the size dynamically.

Comment 10

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screenshots (width 500px)

testcase
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3975

This is caused in Classic theme of Windows XP and Windows 2000.

Comment 11

[Atsushi Sakai]

(In reply to comment #7)
> >+checkbox[disabled="true"]:-moz-system-metric(windows-classic) {
> >+  color: GrayText;
> >+  text-shadow: none;
> >+}
> 
> Why is this rule needed? The other two negate the changes in attachment 347530 [details] [diff] [review],
> but I don't see how this one is relevant.

This rule completely negate Bug 69710.
If text-shadow is not none, this bug occurs.
It might not be needed because Firefox never disable the checkbox, but add-ons may do so.

Comment 12

[Atsushi Sakai]

(In reply to comment #11)
> This rule completely negate Bug 69710
for checkbox in findbar.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Ah, ok, so you're undoing attachment 341424 [details] [diff] [review] as well (I missed that).

Comment 14

[Atsushi Sakai]

From today(20090225)'s trunk nightly, the same symptom occurs even if OS's theme is Luna (XP default).
This new issue can't be fixed by the patch because the patch affects only Classic.

BuildID=20090224033722 SourceStamp=69c86f3acc5a works
BuildID=20090224183225 SourceStamp=0e41c1979d25 fails

I don't know which bug changed this behavior...

Comment 15

[Atsushi Sakai]

(In reply to comment #14)
> From today(20090225)'s trunk nightly, the same symptom occurs even if OS's
> theme is Luna (XP default).
> This new issue can't be fixed by the patch because the patch affects only
> Classic.

Filed Bug 480255 for that.

Comment 16

[Atsushi Sakai]

Comment on attachment 363867 [details] [diff] [review]
Patch v1.0 for branch

Clearing requesting review, pending because of Bug 480255.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

I guess attachment 347530 [details] [diff] [review] makes this patch unnecessary?

Comment 18

[Atsushi Sakai]

(In reply to comment #17)
> I guess attachment 347530 [details] [diff] [review] makes this patch unnecessary?

No, Comment #0 has been reproduced after attachment 347530 [details] [diff] [review] landed.

Comment 19

[Atsushi Sakai]

Comment on attachment 363867 [details] [diff] [review]
Patch v1.0 for branch

Re-requesting review as it is decided to not take bug 445087 on 1.9.1 branch, i.e. 1.9.1 branch does not have bug 480255.

Comment 20

[Atsushi Sakai]

Attachment: Patch v1.0 for latest trunk

Just sync with latest trunk.

Please note that this patch's effect can't be seen on trunk because the same problem is caused by bug 480255 even if this patch is applied.

Comment 21

[Atsushi Sakai]

Attachment: Screenshot of latest Shiretoko nightly

Comment 22

[Atsushi Sakai]

Attachment: Screenshot with patch v1.0

Screenshot of the same situation as comment #21, after patch v1.0 applied

Comment 23

[Atsushi Sakai]

Gavin:
Would you review attachment 363867 [details] [diff] [review]?

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 367967 [details] [diff] [review]
Patch v1.0 for latest trunk

>+/* XXX workaround bug 479957, disable bug 69710 in findbar */
>+checkbox:-moz-system-metric(windows-classic) {
>+  text-shadow: none;
>+}
>+
>+checkbox:-moz-system-metric(windows-classic) > .checkbox-label-box > .checkbox-label {
>+  margin-bottom: 0 !important;
>+}

I'd remove the empty line, to make it clear that the comment covers both rules.

>+checkbox[disabled="true"]:-moz-system-metric(windows-classic) {
>+  color: GrayText;
>+  text-shadow: none;
>+}

This is unnecessary, the checkbox will never be disabled.

Comment 25

[Atsushi Sakai]

Attachment: Patch v1.1 for trunk

Addressed comment.
Carrying over review.

Comment 26

[Atsushi Sakai]

Attachment: Patch for 1.9.1branch

Patch for 1.9.1branch.
It's same as "Patch v1.1 for trunk" except the context is different.

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8715db294e51

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 379338 [details] [diff] [review]
Patch for 1.9.1branch

a191=beltzner

Comment 29

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aca76663da8c

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090525 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090525042828

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090525 Shiretoko/3.5pre (.NET CLR 3.5.30729) ID:20090525041408

Comment 31

[Jim Giordano]

For us mere mortals, now that the status says verified fixed, how long before the fix shows up in a release?

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You can check out the upcoming release candidate for Firefox 3.5. It will be already included there.