Closed
Bug 422132
Opened 17 years ago
Closed 12 years ago
font size 0px on div with scrollbars disables mouse wheel scrolling
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: kapouer_mozilla, Assigned: masayuki)
References
()
Details
(Keywords: regression, testcase)
Attachments
(4 files, 2 obsolete files)
241 bytes,
text/html
|
Details | |
15.65 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
4.42 KB,
patch
|
Details | Diff | Splinter Review | |
1.68 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4
Mouse wheel doesn't work when the scrolling div has font-size:0px;
Please see the test case at the given url.
Reproducible: Always
Steps to Reproduce:
1 - define this html :
<div style="overflow:auto;width:400px;height:200px;font-size:0px;">
<p style="font-size:14px;">long text...</p>
</div>
2 - scroll using mouse wheel over the div
Actual Results:
No scroll using mouse wheel
Expected Results:
able to scroll using mouse wheel
firefox 2 scrolls here, albeit slower than when font-size != 0px
Bug is confirmed in Firefox 3.0.3 under Windows XP. Setting font-size:0 to the scrollable div leads to a number of bugs:
1. Any focused input does not loose focus if user clicks inside such div.
2. Mouse wheel does not scroll div's content.
3. Clicks on the scrollbar scroll buttons do not work.
4. Keyboard up/down arrow keys do not work.
The only possible way to scroll is to click on the area between moving scrollbar and scroll buttons.
Comment 2•15 years ago
|
||
Updated•15 years ago
|
Component: General → Event Handling
Keywords: regression,
testcase
OS: Linux → All
Product: Firefox → Core
QA Contact: general → events
Hardware: x86 → All
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•12 years ago
|
||
I'll fix this bug after bug 422132.
Assignee: nobody → masayuki
Depends on: 719320
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I'll fix this bug after bug 422132.
oops, I meant bug 719320.
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
DoScrollText()'s part is outdenting due to removing the if statement.
The min-line-height pref has magic number "5px". I think that there is no contents whose font-size is 5px or less but the author expects the text should be read. So, I think that such scrollable element is used for other contents such as <img> or something. The magic number 5 is enough small value to change the scroll speed.
Attachment #651722 -
Flags: review?(bugs)
Comment 9•12 years ago
|
||
Comment on attachment 651722 [details] [diff] [review]
Patch
>+ // And also it would computes different delta values from older version.
s/computes/compute/
s/from older/from the older/
Attachment #651722 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Target Milestone: --- → mozilla17
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•12 years ago
|
||
I found a mistake.
If we don't set DeltaAccumulator::mX and DeltaAccumulator::mY, we never check the scroll direction change for DeltaAccumulator::mPendingScrollAmountX and DeltaAccumulator::mPendingScrollAmountY.
First, we should reset them when we detect scroll direction change.
Next, we should set non-zero delta value to mX and mY for recording the scrolling direction when it's not computing lineOrPageDelta values.
Nobody cannot realized this bug from actual behavior, though.
Attachment #652343 -
Flags: review?(bugs)
Updated•12 years ago
|
Attachment #652343 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Sorry backed out for bug 782903 persisting even after the fixup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd626d738af7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Ed Morley [:edmorley] from comment #15)
> Sorry backed out for bug 782903 persisting even after the fixup:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cd626d738af7
Hey, why didn't you just disable the new test? Another bug fix depends on the backed out change.
Comment 17•12 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/22 - 8/27) from comment #16)
> (In reply to Ed Morley [:edmorley] from comment #15)
> > Sorry backed out for bug 782903 persisting even after the fixup:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/cd626d738af7
>
> Hey, why didn't you just disable the new test?
Landing code changes along with a test to verify that the code changes are correct only to disable the test when it fails seems somewhat schizophrenic...
Assignee | ||
Comment 18•12 years ago
|
||
It's too dangerous if the patch is a part of another fix. So, I think that person who backs out some patches due to random orange should confirm if the back out is safe to the author.
Comment 19•12 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/22 - 8/27) from comment #16)
> Hey, why didn't you just disable the new test? Another bug fix depends on
> the backed out change.
Unfortunately it is usual for disabled tests to just stay that way - so if something has landed only in the last day, a backout is preferable to disabling a test - otherwise we have no guarantee it will be resolved. In addition, it's not really my call to make as to whether a patch was acceptable even if it's no longer being tested - that's between the patch author and the reviewer - so it's easier to back and and let that conversation happen prior to relanding. There is also the point that Dao made.
We're just adding way too much new intermittent orange to the tree right now - and we have a culture of on the most part just ignoring it / disabling tests 6 months and 500 failures later. The only way to change this is if we start being a lot more strict on backouts and doing them straight away - particularly given that once a patch has got its foot in the door, it is much harder to back it out a few weeks later - even when we realise that the new orange it caused is actually now a top occurring orange & causing massive frustration/hassle for sheriffs on a daily basis.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/22 - 8/27) from comment #18)
> It's too dangerous if the patch is a part of another fix. So, I think that
> person who backs out some patches due to random orange should confirm if the
> back out is safe to the author.
The backout was only ~24 hours after the landing, didn't have any merge conflicts, I checked the 'blocks' field of this bug (which is empty) & the backout was green on TBPL. Which bug relies on this one? Is there a reason why isn't it listed in the blocks field?
Backouts are such a common part of my day - that there often isn't time to look on IRC (particularly in other cases, where the backout was due to the person not using try etc, so if you mention on IRC they start trying to do in-place fixes rather than backing out) - so I'm not really in the habit of hunting for people on IRC, sorry. Next time for a less black and white case (like this maybe was), I'll try to see if you are around in #developers if that helps? :-)
Assignee | ||
Comment 20•12 years ago
|
||
> > It's too dangerous if the patch is a part of another fix. So, I think that
> > person who backs out some patches due to random orange should confirm if the
> > back out is safe to the author.
>
> The backout was only ~24 hours after the landing, didn't have any merge
> conflicts, I checked the 'blocks' field of this bug (which is empty) & the
> backout was green on TBPL. Which bug relies on this one? Is there a reason
> why isn't it listed in the blocks field?
Bug 782175. I tried it to add the dependency tree, but it failed due to dependency loop detected.
In most cases, the patch looks like working fine even without this bug's patches. But if the scrolling speed is too slow, it could get different result because this bug's patches store unused fractional delta values which will be used at next event handling. The fractional values become more important if scroll speed is slower than usual.
> Backouts are such a common part of my day - that there often isn't time to
> look on IRC (particularly in other cases, where the backout was due to the
> person not using try etc, so if you mention on IRC they start trying to do
> in-place fixes rather than backing out) - so I'm not really in the habit of
> hunting for people on IRC, sorry. Next time for a less black and white case
> (like this maybe was), I'll try to see if you are around in #developers if
> that helps? :-)
It's my fault that I haven't tested the patch enough. I apologize about it.
Anyway, I hope you would contact me before forcibly back something out, or you order me to backout my patches or disable new tests by myself in the bug (If I don't reply it in 24 hours, feel free to do the execution). I'll decide which is better way to backout or disable only new tests in each case. I'm going to be offline due to summer vacation, so, I don't have much time for the deadline. In such situation, accidental backout makes me waste time.
And please note that I'm living in Japan, JST is UTC+9:00, so, IRC isn't a good way to contact me even if I'm there. I usually stay on IRC for getting messages from someone living in different timezone but actually I may be away from my computer.
Assignee | ||
Comment 21•12 years ago
|
||
This patch includes both attachment 651722 [details] [diff] [review] and attachment 652343 [details] [diff] [review] except the new tests.
Attachment #651722 -
Attachment is obsolete: true
Attachment #652343 -
Attachment is obsolete: true
Attachment #653165 -
Flags: review?(bugs)
Assignee | ||
Comment 22•12 years ago
|
||
This is same as the backed out test, I'll attach another patch when I find another approach.
Comment 23•12 years ago
|
||
Comment on attachment 653165 [details] [diff] [review]
Patch without tests
(But we really need to get this tested reliably.)
Attachment #653165 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b7239f4e35
Thanks. I'm testing a possible fix:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fe68b6829faa
Assignee | ||
Comment 25•12 years ago
|
||
Okay, probably, this is the right fix.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=fe68b6829faa
This is a difference between the new tests and test_continuous_wheel_events.html.
Attachment #653225 -
Flags: review?(bugs)
Comment 26•12 years ago
|
||
Comment on attachment 653225 [details] [diff] [review]
Disable smooth scroll in the test
Ah, interesting.
Attachment #653225 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 29•12 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/22 - 8/27) from comment #27)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/863bce1674f5
> https://hg.mozilla.org/integration/mozilla-inbound/rev/12fdeb3b0aba
>
> relanded the tests.
To prevent bugs being closed prematurely in the future (eg if there are still patches to come), add [leave open] to the whiteboard:
https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/863bce1674f5
https://hg.mozilla.org/mozilla-central/rev/12fdeb3b0aba
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 31•10 years ago
|
||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•