Last Comment Bug 422132 - font size 0px on div with scrollbars disables mouse wheel scrolling
: font size 0px on div with scrollbars disables mouse wheel scrolling
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
:
Mentors:
http://kapouer.org/wheel-firefoxb4.html
: 539453 545511 773314 (view as bug list)
Depends on: 719320 782903
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-11 09:28 PDT by Lal
Modified: 2015-04-16 04:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (241 bytes, text/html)
2010-02-10 14:42 PST, Dão Gottwald [:dao]
no flags Details
Patch (19.17 KB, patch)
2012-08-14 05:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
follow up patch (2.14 KB, patch)
2012-08-16 00:27 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
Patch without tests (15.65 KB, patch)
2012-08-19 04:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review
New tests (4.42 KB, patch)
2012-08-19 04:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Disable smooth scroll in the test (1.68 KB, patch)
2012-08-19 17:17 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
bugs: review+
Details | Diff | Splinter Review

Description Lal 2008-03-11 09:28:37 PDT
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
Comment 1 Eugene 2008-10-31 09:39:38 PDT
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 Dão Gottwald [:dao] 2010-02-10 14:42:26 PST
Created attachment 426360 [details]
testcase
Comment 3 Dão Gottwald [:dao] 2010-02-10 14:44:41 PST
*** Bug 545511 has been marked as a duplicate of this bug. ***
Comment 4 Mardeg 2012-07-12 09:21:34 PDT
*** Bug 539453 has been marked as a duplicate of this bug. ***
Comment 5 Mardeg 2012-07-12 09:21:46 PDT
*** Bug 773314 has been marked as a duplicate of this bug. ***
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-07-17 20:25:36 PDT
I'll fix this bug after bug 422132.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-07-17 20:25:58 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I'll fix this bug after bug 422132.

oops, I meant bug 719320.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-14 05:46:01 PDT
Created attachment 651722 [details] [diff] [review]
Patch

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.
Comment 9 Olli Pettay [:smaug] (TPAC) 2012-08-14 08:03:23 PDT
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/
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-14 17:56:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c601428c70b
Comment 11 Ed Morley [:emorley] 2012-08-15 09:48:54 PDT
https://hg.mozilla.org/mozilla-central/rev/5c601428c70b
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-16 00:27:45 PDT
Created attachment 652343 [details] [diff] [review]
follow up patch

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.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-16 18:33:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/41722f2e0c33
Comment 14 Ed Morley [:emorley] 2012-08-17 05:28:10 PDT
https://hg.mozilla.org/mozilla-central/rev/41722f2e0c33
Comment 15 Ed Morley [:emorley] 2012-08-18 15:30:19 PDT
Sorry backed out for bug 782903 persisting even after the fixup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd626d738af7
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-18 19:31:03 PDT
(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 Dão Gottwald [:dao] 2012-08-18 21:52:12 PDT
(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...
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-18 23:49:32 PDT
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 Ed Morley [:emorley] 2012-08-19 03:03:27 PDT
(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? :-)
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 04:00:33 PDT
> > 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.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 04:39:10 PDT
Created attachment 653165 [details] [diff] [review]
Patch without tests

This patch includes both attachment 651722 [details] [diff] [review] and attachment 652343 [details] [diff] [review] except the new tests.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 04:40:56 PDT
Created attachment 653166 [details] [diff] [review]
New tests

This is same as the backed out test, I'll attach another patch when I find another approach.
Comment 23 Olli Pettay [:smaug] (TPAC) 2012-08-19 05:39:47 PDT
Comment on attachment 653165 [details] [diff] [review]
Patch without tests

(But we really need to get this tested reliably.)
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 09:47:34 PDT
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
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 17:17:05 PDT
Created attachment 653225 [details] [diff] [review]
Disable smooth scroll in the test

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.
Comment 26 Olli Pettay [:smaug] (TPAC) 2012-08-19 18:08:59 PDT
Comment on attachment 653225 [details] [diff] [review]
Disable smooth scroll in the test

Ah, interesting.
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2012-08-19 19:47:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/863bce1674f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/12fdeb3b0aba

relanded the tests.
Comment 28 Phil Ringnalda (:philor) 2012-08-19 21:14:04 PDT
https://hg.mozilla.org/mozilla-central/rev/68b7239f4e35
Comment 29 Ed Morley [:emorley] 2012-08-20 01:35:38 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.