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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Atsushi Sakai, Assigned: Atsushi Sakai)

Tracking

(Depends on: 1 bug, {regression, verified1.9.1})

1.9.1 Branch
mozilla1.9.2a1
x86
Windows XP
regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Flags: blocking1.9.1?
(Assignee)

Comment 1

9 years ago
Created attachment 363867 [details] [diff] [review]
Patch v1.0 for branch

"Work around bug 458231" in Bug 69710 triggers this issue.
This is the workaround for the workaround.
Would take a patch, but don't think this issue would hold 1.9.1.  Please re-nom if you disagree.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Assignee)

Updated

9 years ago
Attachment #363867 - Flags: review?(gavin.sharp)
(Assignee)

Comment 3

9 years ago
re-nom blocking.
This is a regression (Fx3.0.x works fine), and it degrades Find Toolbar, which is a major function in Firefox.
Severity: normal → major
Flags: blocking1.9.1- → blocking1.9.1?
Keywords: regression
This appears to be a duplicate of bug 428939.
(Assignee)

Comment 5

9 years ago
(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.
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.
Flags: blocking1.9.1? → blocking1.9.1-
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.
(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".
(Assignee)

Comment 9

9 years ago
(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.
Created attachment 364051 [details]
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.
(Assignee)

Comment 11

9 years ago
(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.
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> This rule completely negate Bug 69710
for checkbox in findbar.
Ah, ok, so you're undoing attachment 341424 [details] [diff] [review] as well (I missed that).
(Assignee)

Comment 14

9 years ago
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...
(Assignee)

Updated

9 years ago
Summary: content is rendered outside the window if width is narrower than findbar → content is rendered outside the window if width is narrower than findbar if OS's theme is Classic
(Assignee)

Comment 15

9 years ago
(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.
(Assignee)

Comment 16

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

Clearing requesting review, pending because of Bug 480255.
Attachment #363867 - Flags: review?(gavin.sharp)
I guess attachment 347530 [details] [diff] [review] makes this patch unnecessary?
(Assignee)

Comment 18

9 years ago
(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.
(Assignee)

Comment 19

9 years ago
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.
Attachment #363867 - Attachment description: Patch v1.0 → Patch v1.0 for branch
Attachment #363867 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

9 years ago
Created attachment 367967 [details] [diff] [review]
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.
(Assignee)

Comment 21

9 years ago
Created attachment 370397 [details]
Screenshot of latest Shiretoko nightly
(Assignee)

Comment 22

9 years ago
Created attachment 370399 [details]
Screenshot with patch v1.0

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

Comment 23

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

Updated

9 years ago
Attachment #363867 - Flags: review?(gavin.sharp) → review?(dao)

Updated

9 years ago
Attachment #367967 - Flags: review+
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.

Updated

9 years ago
Attachment #363867 - Flags: review?(dao) → review+
(Assignee)

Comment 25

9 years ago
Created attachment 379337 [details] [diff] [review]
Patch v1.1 for trunk

Addressed comment.
Carrying over review.
Assignee: nobody → dev-null
Attachment #363867 - Attachment is obsolete: true
Attachment #367967 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #379337 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

9 years ago
Created attachment 379338 [details] [diff] [review]
Patch for 1.9.1branch

Patch for 1.9.1branch.
It's same as "Patch v1.1 for trunk" except the context is different.
Attachment #379338 - Flags: approval1.9.1?
http://hg.mozilla.org/mozilla-central/rev/8715db294e51
Severity: major → normal
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → 1.9.1 Branch
Comment on attachment 379338 [details] [diff] [review]
Patch for 1.9.1branch

a191=beltzner
Attachment #379338 - Flags: approval1.9.1? → approval1.9.1+

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aca76663da8c
Keywords: checkin-needed → fixed1.9.1
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1

Comment 31

9 years ago
For us mere mortals, now that the status says verified fixed, how long before the fix shows up in a release?
You can check out the upcoming release candidate for Firefox 3.5. It will be already included there.
You need to log in before you can comment on or make changes to this bug.