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]
:
: Andrew Overholt [:overholt]
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]
bugs: review+
Details | Diff | Splinter Review
follow up patch (2.14 KB, patch)
2012-08-16 00:27 PDT, Masayuki Nakano [:masayuki]
bugs: review+
Details | Diff | Splinter Review
Patch without tests (15.65 KB, patch)
2012-08-19 04:39 PDT, Masayuki Nakano [:masayuki]
bugs: review+
Details | Diff | Splinter Review
New tests (4.42 KB, patch)
2012-08-19 04:40 PDT, Masayuki Nakano [:masayuki]
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]
bugs: review+
Details | Diff | Splinter Review

Description User image 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 User image 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 User image Dão Gottwald [:dao] 2010-02-10 14:42:26 PST
Created attachment 426360 [details]
testcase
Comment 3 User image Dão Gottwald [:dao] 2010-02-10 14:44:41 PST
*** Bug 545511 has been marked as a duplicate of this bug. ***
Comment 4 User image Mardeg 2012-07-12 09:21:34 PDT
*** Bug 539453 has been marked as a duplicate of this bug. ***
Comment 5 User image Mardeg 2012-07-12 09:21:46 PDT
*** Bug 773314 has been marked as a duplicate of this bug. ***
Comment 6 User image Masayuki Nakano [:masayuki] 2012-07-17 20:25:36 PDT
I'll fix this bug after bug 422132.
Comment 7 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Masayuki Nakano [:masayuki] 2012-08-14 17:56:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c601428c70b
Comment 11 User image Ed Morley [:emorley] 2012-08-15 09:48:54 PDT
https://hg.mozilla.org/mozilla-central/rev/5c601428c70b
Comment 12 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 2012-08-16 18:33:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/41722f2e0c33
Comment 14 User image Ed Morley [:emorley] 2012-08-17 05:28:10 PDT
https://hg.mozilla.org/mozilla-central/rev/41722f2e0c33
Comment 15 User image 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 25 User image Masayuki Nakano [:masayuki] 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2012-08-19 18:08:59 PDT
Comment on attachment 653225 [details] [diff] [review]
Disable smooth scroll in the test

Ah, interesting.
Comment 28 User image Phil Ringnalda (:philor) 2012-08-19 21:14:04 PDT
https://hg.mozilla.org/mozilla-central/rev/68b7239f4e35
Comment 29 User image 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.