Closed Bug 422132 Opened 16 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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: kapouer_mozilla, Assigned: masayuki)

References

()

Details

(Keywords: regression, testcase)

Attachments

(4 files, 2 obsolete files)

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.
Attached file testcase
Component: General → Event Handling
Keywords: regression, testcase
OS: Linux → All
Product: Firefox → Core
QA Contact: general → events
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
I'll fix this bug after bug 422132.
Assignee: nobody → masayuki
Depends on: 719320
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I'll fix this bug after bug 422132.

oops, I meant bug 719320.
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/5c601428c70b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached patch follow up patch (obsolete) — Splinter Review
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)
Attachment #652343 - Flags: review?(bugs) → review+
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 → ---
(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.
(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...
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.
(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? :-)
> > 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.
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)
Attached patch New testsSplinter Review
This is same as the backed out test, I'll attach another patch when I find another approach.
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+
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 on attachment 653225 [details] [diff] [review]
Disable smooth scroll in the test

Ah, interesting.
Attachment #653225 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/68b7239f4e35
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(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 → ---
https://hg.mozilla.org/mozilla-central/rev/863bce1674f5
https://hg.mozilla.org/mozilla-central/rev/12fdeb3b0aba
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: