So at a discussion the CSS WG is having with Japanese publishers today (with some brief minutes at https://log.csswg.org/irc.w3.org/css/2017-04-19/#e798766), one of the things that came up is that people are hesitant to use vertical writing-mode at the top level of the page because the resulting horizontal scrolling doesn't work well if the user has a mousewheel, which has efficient scrolling only in one direction. It seems the desired user behavior is that if the user has a mousewheel (i.e., a scrolling device that works only vertically and not horizontally), then if the page uses vertical writing-mode at the top level (i.e., a vertical writing mode on *body* (or the root if there's no body) (which should the way we decide which direction of overflow gets paginated), then the mousewhere should translate to horizontal scrolling rather than vertical.

Unfortunately, we cannot know active pointing device supports horizontal scroll or not. How about to scroll contents with any direction wheel events if overflow occurs only vertical or horizontal direction? # I don't think that we should touch the attribute values of WheelEvent for compatibility with web apps, so, I assume that your suggestion here is just to change the default action of wheel events.

Yes, I think we're talking about only the default action. Not sure about scrolling either direction.

Is this something we shall prioritize and ship in the next release(s) or so?

(In reply to Masayuki Nakano [:masayuki] from comment #1) > How about to scroll contents with any direction wheel events if overflow > occurs only vertical or horizontal direction? So a wide page like http://joshworth.com/dev/pixelspace/pixelspace_solarsystem.html would scroll (horizontally) in response to (vertical) scrollwheel events. That seems pretty reasonable to me. The other option is to just ignore the scrollwheel if there's no overflow in that axis, but that doesn't serve any useful purpose. (Actually, I suspect that particular page may implement this behavior itself anyway -- it scrolls horizontally in response to either vertical OR horizontal touchpad-swipes for me.) Another kind of horizontal-mode example would be a "reader" type of page that formats content into a series of columns that fit within the visible height of the page (so no vertical scrolling), and extend beyond it horizontally. Again, allowing the (vertical-oriented) scrollwheel to scroll the sequence of columns (horizontally) seems fine IMO. So I'd be in favor of implementing this, at least to see how people like it in practice.

(In reply to Jonathan Kew (:jfkthame) from comment #4) > The other option is to just ignore the scrollwheel > if there's no overflow in that axis, but that doesn't serve any useful > purpose. I don't understand what you mean here. Currently, we ignore the delta values which cannot scroll to the direction. > So I'd be in favor of implementing this, at least to see how people like it > in practice. Please be careful about that if the pointing device is a touchpad supporting both direction to scroll, perhaps, lower delta should be ignored. Otherwise, it causes unexpected scroll to the user. I think that WidgetEvent should have |double GetPreferredDelta() const| which should be same logic as GetPreferredIntDelta() but refers mDeltaX and mDelatY. And it should be used only when the scroll target of the wheel event is overflown to one direction. And perhaps, the direction to scroll with different axis delta value should be customizable with pref. Note that currently, default action of wheel event is handled in EventStateManager::DoScrollText() for normal mode. If APZC is available, it's handled in AsyncPanZoomController::OnScrollWheel().

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #1) > Unfortunately, we cannot know active pointing device supports horizontal > scroll or not. > > How about to scroll contents with any direction wheel events if overflow > occurs only vertical or horizontal direction? I would like to take this bug, and this feature is what I am looking forward to have for daily use of my visitors and mine for many years because I am also a website designer interested in vertical writing pages. I agree with Masayuki, if a page overflows only in one direction, it's rational to treat one direction scroll as scroll in that direction no matter which direction the wheel is in, although there might still be a tricky situation for this solution: Let's imagine a user is using a pointing device with only one wheel, and is browsing a vertical writing-mode page in a narrow viewport with one long main article and one side content. It's likely overflow occur in both the two directions, but the user obviously would like the wheel to scroll horizontally. Unfortunately, there is no "one size fits all" solution for now. This solution just suits most situations best.

There is one more thing to mention, if a vertical wheel triggers horizontal scroll, we should further distinguish behaviour between vertical-rl/sideways-rl and vertical-lr/sideways-lr. Scrolling the wheel up should trigger scroll rightwardsin vertical-rl/sideways-rl and leftwards in vertical-lr/sideways-lr, and vice versa for scrolling the wheel down

I am going to work on this. But before taking actual steps in it, I would like to discuss the detailed UE solution with you to make sure what I am going to work on is based on a consistent understanding of all of us. Here is my proposal, which introduces a new concept called "smart wheel scroll": For a smart wheel scroll, if all of the four conditions are met: 1. DeltaZ is zero; 2. There is only one non-zero value between DeltaX and DeltaY; 3. There is only one direction for the target that overflows and is scrollable with wheel; 4. The direction described in Condition 1 is not the same as the one described in Condition 2; then handle the default action *AS IF* deltas were modified by the following rules: For horizontal or vertical-LTR writing mode: If DeltaY != 0, let DeltaX = DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = DeltaX, let DeltaX = 0 For vertical-RTL writing mode: If DeltaY != 0, let DeltaX = -DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = -DeltaX, let DeltaX = 0 And I propose to not only implement this smart scroll for the page's top-level widget, but for all the targets within the page. Smart scrolling is likely to be preferred by most of the users who have only one vertical wheel on their mice, especially for single-line text controls with long text and vertical-writing pages, and I think it does no harm even if they are browsing the horizontal-writing pages, so I propose to set the default action of wheel scroll work this way, and add a new pref value for old regular scrolling if the user does't want this feature. I am looking forward to hearing your opinions on the proposal. Thanks.

4. The direction described in Condition 1 is not the same as the one described in Condition 2; Sorry for typo. Condition 1 meant Condition 2, and Condition 2 meant Condition 3.

A new definition for smart wheel scroll, with typos corrected. For a smart wheel scroll, if all of the four conditions are met: 1. DeltaZ is zero; 2. There is only one non-zero value between DeltaX and DeltaY; 3. There is only one direction for the target that overflows and is scrollable with wheel; 4. The direction described in Condition 2 is not the same as(i.e. orthogonal to) the one described in Condition 3; then handle the default action *AS IF* deltas were modified by the following rules: For horizontal or vertical-LTR writing mode: If DeltaY != 0, let DeltaX = DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = DeltaX, let DeltaX = 0 For vertical-RTL writing mode: If DeltaY != 0, let DeltaX = -DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = -DeltaX, let DeltaX = 0

I think it's more useful for Masayuki and Jonathan to review that than for me to do so.

Please ignore the descriptions for smart wheel scroll in comment 9 and comment 11. Here is the latest edition: For a smart wheel scroll, if all of the four conditions are met: 1. DeltaZ is zero; 2. There is only one non-zero value between DeltaX and DeltaY; 3. There is only one direction for the target that overflows and is scrollable with wheel; 4. The direction described in Condition 2 is not the same as(i.e. orthogonal to) the one described in Condition 3; then handle the default action *AS IF* deltas were modified by the following rules: For a target either in horizontal-LTR writing mode(E.g. |writing-mode| is "horizontal-tb" and |direction| is "ltr" in CSS) or in vertical-LTR writing mode(E.g. |writing-mode| is "vertical-lr" in CSS language): If DeltaY != 0, let DeltaX = DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = DeltaX, let DeltaX = 0 For a target in horizontal-RTL writing mode(E.g. |writing-mode| is "horizontal-tb" and |direction| is "rtl" in CSS): If DeltaY != 0, let DeltaX = -DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = DeltaX, let DeltaX = 0 For a target in vertical-RTL writing mode(E.g. |writing-mode| is "vertical-rl" in CSS): If DeltaY != 0, let DeltaX = -DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = -DeltaX, let DeltaX = 0

Yeah, I'll review it. However, I don't have much today.

Sorry, again ignore comment 13. I think comment 13 again sounds wrong. This one is the more reasonable then comment 13: For a target either in horizontal-LTR writing mode(E.g. |writing-mode| is "horizontal-tb" and |direction| is "ltr" in CSS) or in vertical-LTR writing mode(E.g. |writing-mode| is "vertical-lr" in CSS): If DeltaY != 0, let DeltaX = DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = DeltaX, let DeltaX = 0 For a target either in horizontal-RTL writing mode(E.g. |writing-mode| is "horizontal-tb" and |direction| is "rtl" in CSS) or in vertical-RTL writing mode(E.g. |writing-mode| is "vertical-rl" in CSS): If DeltaY != 0, let DeltaX = -DeltaY, let DeltaY = 0 If DeltaX != 0, let DeltaY = -DeltaX, let DeltaX = 0

I uploaded an HTML attachment file so that we can conceptualize "smart scroll" in an intuitive way. If it's implemented, it should responses to wheel scroll according to descriptions on a light green background.

If the idea seems fine, please assign this to me.

Sounds reasonable. But perhaps, you can define the behavior simpler if you do DeltaFoo * -1 as an additional step only in some direction cases. Anyway, I have two concerns: 1. On Windows and Linux, we receive vertical and horizontal wheel events separately when user tries to scroll diagonally. Do you think that user should just disable the new behavior from pref if user likes diagonal scroll? 2. If web pages wants to block box horizontally, there are two ways, one is to use float, flex or something with horizontal layout, but the other is to use vertical writing-mode. If they used the latter and specifying horizontal writing-mode in the blocks, your implementation must break compatibility with the other browsers. So, I still think that it *might* be safe to honor only root or body element's writing mode to decide the wheel direction.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18) > Sounds reasonable. But perhaps, you can define the behavior simpler if you > do DeltaFoo * -1 as an additional step only in some direction cases. > > Anyway, I have two concerns: > > 1. On Windows and Linux, we receive vertical and horizontal wheel events > separately when user tries to scroll diagonally. Do you think that user > should just disable the new behavior from pref if user likes diagonal scroll? Yes, I think so. It is always good to be configurable though I believe most users will prefer the smart scrolling feature than diagonal scrolling. > 2. If web pages wants to block box horizontally, there are two ways, one is > to use float, flex or something with horizontal layout, but the other is to > use vertical writing-mode. If they used the latter and specifying horizontal > writing-mode in the blocks, your implementation must break compatibility > with the other browsers. So, I still think that it *might* be safe to honor > only root or body element's writing mode to decide the wheel direction. Firstly, for semantical rightness, the former method(|float|, |flex|, |grid| or even |table| groups) is quite better then the latter one(|writing-mode|) for layouting boxes horizontally. Secondly, even if the user decides to use the latter method, its rarely seen and unreasonable for the user to choose |vertical-rl| for Latin-based or any other left-to-right writing languages, or choose |vertical-lr| for Arabic/Hebrew-based languages. Because if so, when viewing in a narrow viewport, the horizontal scrollbar will start at the unhoped edge(i.e. at the right edge on an English page, or start at left edge on an Arabic page). so not to say anything else, this is mistaken on its own. If both the two above bad designs happen, it becomes a pretty wrong design. So my personal opinion is if the user makes mistakes, let them just be mistakes, it's not a blame for a browser to judge a correction for those. Right now, no browser as far as I know has implemented a similar feature, so I don't think there may be any concerns for breaking compatibility with the other browsers. Or do you know of any browsers with a similar feature?

(In reply to Zhang Junzhi from comment #19) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18) > > Sounds reasonable. But perhaps, you can define the behavior simpler if you > > do DeltaFoo * -1 as an additional step only in some direction cases. > > > > Anyway, I have two concerns: > > > > 1. On Windows and Linux, we receive vertical and horizontal wheel events > > separately when user tries to scroll diagonally. Do you think that user > > should just disable the new behavior from pref if user likes diagonal scroll? > > Yes, I think so. It is always good to be configurable though I believe most > users will prefer the smart scrolling feature than diagonal scrolling. I think that most users don't look for a prefs to disable something when they meet annoying behavior. Instead, some of them try to switch their default browser. So, we need to be careful for this issue. I wonder, at least on Windows, wheel events are in the message queue. Cannot we check the following WM_MOUSE(H)SCROLL message and mark WidgetWheelEvent as whether the user is scrolling diagonally with a new bool member? > > 2. If web pages wants to block box horizontally, there are two ways, one is > > to use float, flex or something with horizontal layout, but the other is to > > use vertical writing-mode. If they used the latter and specifying horizontal > > writing-mode in the blocks, your implementation must break compatibility > > with the other browsers. So, I still think that it *might* be safe to honor > > only root or body element's writing mode to decide the wheel direction. > > Firstly, for semantical rightness, the former method(|float|, |flex|, |grid| > or even |table| groups) is quite better then the latter one(|writing-mode|) > for layouting boxes horizontally. Yes, it is. However, we always struggle with correctness vs. actual web sites. We should not assume that there is only few web sites which are designed wrongly. Note that writing-mode has an advantage. It's supported by IE too. So, somebody could take it as a dirty hack. > Secondly, even if the user decides to use the latter method, its rarely seen > and unreasonable for the user to choose |vertical-rl| for Latin-based or any > other left-to-right writing languages, or choose |vertical-lr| for > Arabic/Hebrew-based languages. Because if so, when viewing in a narrow > viewport, the horizontal scrollbar will start at the unhoped edge(i.e. at > the right edge on an English page, or start at left edge on an Arabic page). > so not to say anything else, this is mistaken on its own. Indeed. But I have another idea for using vertical layout whose block direction is different from the inline direction of its parent. E.g., even if the parent language is left-to-right, they may want to show first item at rightmost in come cases. If they sort the content by timeline and want to put the latest block to the rightmost. However, in this case, the scroll direction must be correct in semantics, as you say. So, must not be a problem. > If both the two above bad designs happen, it becomes a pretty wrong design. > So my personal opinion is if the user makes mistakes, let them just be > mistakes, it's not a blame for a browser to judge a correction for those. I don't think this is good reason to use hacky behavior. First, you say "user" here, but they're "author", "designer" or "developer", so, not *our* user. If we do some hacky behavior for our users but that causes making some damage of UX on some web sites, actual victims are our users, and they may switch from Firefox to another browser. So, we need to be careful to do something hacky behavior even if it's useful in most cases. > Right now, no browser as far as I know has implemented a similar feature, so > I don't think there may be any concerns for breaking compatibility with the > other browsers. Or do you know of any browsers with a similar feature? They do NOT behave like as you'll implement. That means that, web pages are designed for the traditional behavior. So, we need to minimize the risk even if we take "better" hacky behavior.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20) > I wonder, at least on Windows, wheel events are in the message queue. Cannot > we check the following WM_MOUSE(H)SCROLL message and mark WidgetWheelEvent > as whether the user is scrolling diagonally with a new bool member? Hmm, so besides giving a pref to disable this feature, we should also check for diagonal scrolling events. but this will involve with OS-dependent coding. I am not familar with development on Windows since I haven't written code for Windows for many years. However, I'll have a try. > They do NOT behave like as you'll implement. That means that, web pages are > designed for the traditional behavior. So, we need to minimize the risk even > if we take "better" hacky behavior. So if we decide to base the scroll direction only on page's overall horizontal and vertical directions, we need to formally conceptualize what a page's overall horizontal and vertical directions are. I suggest <body> is prioritized, that is, the scroll direction depends on the computed value of |writing-mode| for <body>, if the <body> is missing, then it falls back to depend on the computed value of |writing-mode| for the root element. This is a much simpler conceptualization. For a much more complicated conceptualization: when the <body>'s writing-mode is not the same as the root element's, <body> isn't always guaranteed to be ACTUALLY OVERALL element, because |display| properties of elements outside of <body> such as <head>, or child elements in <head> may not be |none|, which is very weird but at least technically possible. Moreover, making |display| properties of elements outside of <body> a value other than |none| does not necessarily rule out the possibility for <body> to be ACTUALLY OVERALL, for example, if the display of <head> is not |none|, but the |display| of all children of <head> are still |none|, in that case, <body> is still the actually overall element. Should we consider such weird situations which may complicate the implementation? I prefer the simpler one.

> They do NOT behave like as you'll implement. That means that, web pages are > designed for the traditional behavior. So, we need to minimize the risk even > if we take "better" hacky behavior. Personally speaking, my favorite solution is still current-writing-mode-for-current-target. If the scrollbar starts at the rightside, then actually for what reason does it bother the user to scroll wheel downwards to go leftwards for that target? Or vice verse, scroll wheel downwards to go rightwards if the scrollbar starts at the leftside? I think it may not be counter-intuitive at its worst.

> They do NOT behave like as you'll implement. That means that, web pages are > designed for the traditional behavior. So, we need to minimize the risk even > if we take "better" hacky behavior. One thing I forgot to mention: for horizontalized scrolling which is implemented by you(Shift+vertical wheel), I think it's better to still keep the scroll direction unrelated to the horizontal writing direction, ensuring it still compatible with most of the browsers, we can just let a pure vertical wheel-scroll without any modifier to be writing-mode-related.

(In reply to Zhang Junzhi from comment #21) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20) > > > I wonder, at least on Windows, wheel events are in the message queue. Cannot > > we check the following WM_MOUSE(H)SCROLL message and mark WidgetWheelEvent > > as whether the user is scrolling diagonally with a new bool member? > > Hmm, so besides giving a pref to disable this feature, we should also check > for diagonal scrolling events. but this will involve with OS-dependent > coding. I am not familar with development on Windows since I haven't written > code for Windows for many years. However, I'll have a try. Fortunately, we are NOT handling WM_MOUSE(H)WHEEL message synchronously. For making the code path simpler even with complicated cases, we post our internal mousescroll message first: https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/WinMouseScrollHandler.cpp#476-479 Then, we will handle the internal wheel message at next event loop (i.e., giving almost highest priority): https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/nsAppShell.cpp#488-501 Finally, we dispatch eWheel event from here: https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/WinMouseScrollHandler.cpp#600,645-646,650 So, I *guess* that when we handle the internal wheel message, next WM_MOUSE(H)WHEEL message may be already in the message queue. You can check it with PeekMessageW(): https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms644943(v=vs.85).aspx On the other hand, we should provide new pref to disable the hack completely. > > They do NOT behave like as you'll implement. That means that, web pages are > > designed for the traditional behavior. So, we need to minimize the risk even > > if we take "better" hacky behavior. > > So if we decide to base the scroll direction only on page's overall > horizontal and vertical directions, we need to formally conceptualize what a > page's overall horizontal and vertical directions are. I suggest <body> is > prioritized, that is, the scroll direction depends on the computed value of > |writing-mode| for <body>, if the <body> is missing, then it falls back to > depend on the computed value of |writing-mode| for the root element. This is > a much simpler conceptualization. > > For a much more complicated conceptualization: when the <body>'s > writing-mode is not the same as the root element's, <body> isn't always > guaranteed to be ACTUALLY OVERALL element, because |display| properties of > elements outside of <body> such as <head>, or child elements in <head> may > not be |none|, which is very weird but at least technically possible. > Moreover, making |display| properties of elements outside of <body> a value > other than |none| does not necessarily rule out the possibility for <body> > to be ACTUALLY OVERALL, for example, if the display of <head> is not |none|, > but the |display| of all children of <head> are still |none|, in that case, > <body> is still the actually overall element. Should we consider such weird > situations which may complicate the implementation? > > I prefer the simpler one. I think that you don't need to worry about the <body> element vs. outside of <body> elements because our layout code has "root scrollable frame": https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/events/EventStateManager.cpp#3403-3404 It must be reasonable use its writing-mode value. (In reply to Zhang Junzhi from comment #22) > > They do NOT behave like as you'll implement. That means that, web pages are > > designed for the traditional behavior. So, we need to minimize the risk even > > if we take "better" hacky behavior. > > Personally speaking, my favorite solution is still > current-writing-mode-for-current-target. How about to separate a pref to enable new behavior as one is to enable/disable the feature completely and the other is to use root scrollable frame's direction always or use current target's direction. Then, we can test both behavior before enabling the behavior in the default settings. Anyway, are there some lists of web sites which use writing-mode? I'd like to see actual web sites which use writing-mode. FYI: One possible bad scenario is: <body style="writing-mode: horizontal-tb"> ... <div style="width: 500px; height: 500px; overflow: auto;"> <div style="width: 3000px; height: 500px; writing-mode: vertical-rl;"> <p>...</p> <p>...</p> </div> </div>

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #24) > (In reply to Zhang Junzhi from comment #21) > > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20) > > > > > I wonder, at least on Windows, wheel events are in the message queue. Cannot > > > we check the following WM_MOUSE(H)SCROLL message and mark WidgetWheelEvent > > > as whether the user is scrolling diagonally with a new bool member? > > > > Hmm, so besides giving a pref to disable this feature, we should also check > > for diagonal scrolling events. but this will involve with OS-dependent > > coding. I am not familar with development on Windows since I haven't written > > code for Windows for many years. However, I'll have a try. > > Fortunately, we are NOT handling WM_MOUSE(H)WHEEL message synchronously. For > making the code path simpler even with complicated cases, we post our > internal mousescroll message first: > https://searchfox.org/mozilla-central/rev/ > 33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/ > WinMouseScrollHandler.cpp#476-479 > Then, we will handle the internal wheel message at next event loop (i.e., > giving almost highest priority): > https://searchfox.org/mozilla-central/rev/ > 33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/nsAppShell.cpp#488- > 501 > Finally, we dispatch eWheel event from here: > https://searchfox.org/mozilla-central/rev/ > 33cc9e0331da8d9ff3750f1e68d72d61201176cb/widget/windows/ > WinMouseScrollHandler.cpp#600,645-646,650 > > So, I *guess* that when we handle the internal wheel message, next > WM_MOUSE(H)WHEEL message may be already in the message queue. > You can check it with PeekMessageW(): > https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms644943(v=vs.85). > aspx > Thank you for the information. That sounds a fortunate news. Hopefully, this two-queue mechanism is "slow enough to be able to" wait for the latter wheel event in diagonal action. Sometimes we are weirdly willing to see a programme "run a bit slower". > > On the other hand, we should provide new pref to disable the hack completely. > > > > They do NOT behave like as you'll implement. That means that, web pages are > > > designed for the traditional behavior. So, we need to minimize the risk even > > > if we take "better" hacky behavior. > > > > So if we decide to base the scroll direction only on page's overall > > horizontal and vertical directions, we need to formally conceptualize what a > > page's overall horizontal and vertical directions are. I suggest <body> is > > prioritized, that is, the scroll direction depends on the computed value of > > |writing-mode| for <body>, if the <body> is missing, then it falls back to > > depend on the computed value of |writing-mode| for the root element. This is > > a much simpler conceptualization. > > > > For a much more complicated conceptualization: when the <body>'s > > writing-mode is not the same as the root element's, <body> isn't always > > guaranteed to be ACTUALLY OVERALL element, because |display| properties of > > elements outside of <body> such as <head>, or child elements in <head> may > > not be |none|, which is very weird but at least technically possible. > > Moreover, making |display| properties of elements outside of <body> a value > > other than |none| does not necessarily rule out the possibility for <body> > > to be ACTUALLY OVERALL, for example, if the display of <head> is not |none|, > > but the |display| of all children of <head> are still |none|, in that case, > > <body> is still the actually overall element. Should we consider such weird > > situations which may complicate the implementation? > > > > I prefer the simpler one. > > I think that you don't need to worry about the <body> element vs. outside of > <body> elements because our layout code has "root scrollable frame": > https://searchfox.org/mozilla-central/rev/ > 33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/events/EventStateManager. > cpp#3403-3404 > It must be reasonable use its writing-mode value. > > (In reply to Zhang Junzhi from comment #22) > > > They do NOT behave like as you'll implement. That means that, web pages are > > > designed for the traditional behavior. So, we need to minimize the risk even > > > if we take "better" hacky behavior. > > > > Personally speaking, my favorite solution is still > > current-writing-mode-for-current-target. > > How about to separate a pref to enable new behavior as one is to > enable/disable the feature completely and the other is to use root > scrollable frame's direction always or use current target's direction. Then, > we can test both behavior before enabling the behavior in the default > settings. Current, there are two default actions for scrolling: scrolling contents and scrolling contents horizontalizedlly. I am going to extend them into four: scrolling contents smartly as per the current-target's writing-mode scrolling contents smartly as per the "root scrollable frame"'s writing-mode scrolling contents scrolling contents horizontalizedlly > > Anyway, are there some lists of web sites which use writing-mode? I'd like > to see actual web sites which use writing-mode. > It's very unfortunate, I haven't even heard of a single vertical-writing website that is in official run. I think it's this bug, as well as the lack of such feature in all other browsers, that actually makes the reality of the day. Bad scrolling UX is enough to stop web developers interested in it from going ahead. Horizontal-RTL websites are easily found because almost all of the Arabic websites are in this mode. But right now I haven't found a nested scrollable element in this kind of websites. At least we can make the page have a horizontal scrollbar by narrowing the browser's window, and then test the page's horizontal scrolling behavior. > FYI: One possible bad scenario is: > > <body style="writing-mode: horizontal-tb"> > ... > <div style="width: 500px; height: 500px; overflow: auto;"> > <div style="width: 3000px; height: 500px; writing-mode: vertical-rl;"> > <p>...</p> > <p>...</p> > </div> > </div> The div will have a horizontal scrollbar starting at rightside, so from my point of view, scrolling wheel downwards unconsciously means to continue reading. It doesn't look awkward for me that the content scrolls leftwards by a downward wheel. But I will provide a example to test it after I have made both the two implementations(for-current-target and for-root-scrollable)

To Masayuki: So the smart scrolling comes down to go with two solutions: Scrolling per se the writing-mode of the current target. I called it S1. Scrolling per se the writing-mode of root scrollable element. I called it S2. I have scheduled serveral commits for the patch: Commit 1: the APZ part for S1 (along with basic code, such as common classes, functions etc.) Commit 2: the APZ part for S2 Commit 3: the non-APZ part for both S1 and S2 Commit 4: diagonal scrolling detection Commit 5: testing part The divided commits are not only for incremental reviews, but also for incremental UX testing. Here is Commit 1. ----------------------------------- To Kartikaya: The previous discussions are somewhat lengthy, please go to comment 11, comment 15 and comment 16 (and its attachment) for a quick look, this commit just implements the feature mentioned in these comments. ----------------------------------- To both: Thank you for your review.

The expected behaviour should be seen on attachment 8955540 [details] , after patching this commit.

Just as a heads-up, it'll probably take me a couple of days to get to this review since it's so large and will need some time to think about. I read through the comments and discussion on this bug and overall the plan seems reasonable, but I'm confused about why S1 and S2 are different. Shouldn't we always just use the writing-mode of the target scrollable frame?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29) > Just as a heads-up, it'll probably take me a couple of days to get to this > review since it's so large and will need some time to think about. Yes, I appreciate your review efforts. > I read > through the comments and discussion on this bug and overall the plan seems > reasonable, but I'm confused about why S1 and S2 are different. Shouldn't we > always just use the writing-mode of the target scrollable frame? Masayuki raised the issue that some designers may use writing-mode as an alternative to |float|, |table|, |grid|, etc. for layouting boxes horizontally, so just using the root scrollable frame's writing-mode might be safer, see comment 24 . The two different solutions will both be implemented, and they can be switched by pref(Which will be implemented in a future commit). Currently, only S1 has been implemented.

It might be a good idea to deploy some telemetry to see how often this scenario (with the writing-mode for horizontal layout) occurs in the wild, and then we can just pick S1 or S2 based on whether or not it is frequent. Having to implement both will add more complexity which might be worse in the long-term.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #31) > It might be a good idea to deploy some telemetry to see how often this > scenario (with the writing-mode for horizontal layout) occurs in the wild, > and then we can just pick S1 or S2 based on whether or not it is frequent. > Having to implement both will add more complexity which might be worse in > the long-term. Currently the complexity it may increase is unknown yet. I haven't begun this part. If it won't change much code, it's fine, otherwise, we may decide to choose one.

(In reply to Zhang Junzhi from comment #25) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #24) > > (In reply to Zhang Junzhi from comment #22) > > > > They do NOT behave like as you'll implement. That means that, web pages are > > > > designed for the traditional behavior. So, we need to minimize the risk even > > > > if we take "better" hacky behavior. > > > > > > Personally speaking, my favorite solution is still > > > current-writing-mode-for-current-target. > > > > How about to separate a pref to enable new behavior as one is to > > enable/disable the feature completely and the other is to use root > > scrollable frame's direction always or use current target's direction. Then, > > we can test both behavior before enabling the behavior in the default > > settings. > > Current, there are two default actions for scrolling: scrolling contents and > scrolling contents horizontalizedlly. I am going to extend them into four: > scrolling contents smartly as per the current-target's writing-mode > scrolling contents smartly as per the "root scrollable frame"'s writing-mode > scrolling contents > scrolling contents horizontalizedlly I don't think that's good design of the pref since it's too complicated. I think that existing pref values are treated as "scroll contents along axis of delta values" and "scroll contents along x-axis with deltaY". Then, you should add new prefs "treat x-axis as inline flow direction, y-axis as block flow direction" and "always use root scrollable frame's flow direction". > > Anyway, are there some lists of web sites which use writing-mode? I'd like > > to see actual web sites which use writing-mode. > > > > It's very unfortunate, I haven't even heard of a single vertical-writing > website that is in official run. I think it's this bug, as well as the lack > of such feature in all other browsers, that actually makes the reality of > the day. Bad scrolling UX is enough to stop web developers interested in it > from going ahead. I don't want list of bad website list. I just want to see actual web sites which use writing-mode a lot. > Horizontal-RTL websites are easily found because almost all of the Arabic > websites are in this mode. But right now I haven't found a nested scrollable > element in this kind of websites. At least we can make the page have a > horizontal scrollbar by narrowing the browser's window, and then test the > page's horizontal scrolling behavior. Yes, it must be rare web sites to have nested scrollable elements. Therefore, isn't it enough to refer root scrollable frame's writing-mode even in nested scrollable elements? I worry about that it must make users confused if scroll direction is changed only in some scrollable elements. > > FYI: One possible bad scenario is: > > > > <body style="writing-mode: horizontal-tb"> > > ... > > <div style="width: 500px; height: 500px; overflow: auto;"> > > <div style="width: 3000px; height: 500px; writing-mode: vertical-rl;"> > > <p>...</p> > > <p>...</p> > > </div> > > </div> > > The div will have a horizontal scrollbar starting at rightside, so from my > point of view, scrolling wheel downwards unconsciously means to continue > reading. It doesn't look awkward for me that the content scrolls leftwards > by a downward wheel. But I will provide a example to test it after I have > made both the two implementations(for-current-target and for-root-scrollable) FYI: When I hacked a lot of wheel event handling, I saw some web pages which has a scrollable <div> element as child of non-scrollable <body> element. So, we need to assume that there are a lot of web pages.

Comment on attachment 8957130 [details] Bug 1358017 - Part 1: Introduces a new feature: smart wheel scrolling(only as per the current-target's writing-mode) https://reviewboard.mozilla.org/r/226066/#review232652 ::: browser/app/profile/firefox.js:626 (Diff revision 1) > // 0: Nothing happens > -// 1: Scrolling contents > +// 1: Scrolling contents smartly, that is, you can scroll a target in the > +// direction orthogonal to the wheel if there's no scrollbar in the wheel's > +// direction but a scrollbar in the wheel's orthogonal direction > // 2: Go back or go forward, in your history > // 3: Zoom in or out. > // 4: Treat vertical wheel as horizontal scroll > +// 5: Scroll contents So, I think that you should add new two prefs. Instead of adding new values. One is for making deltaX/deltaY treated as inline/block flow direction. The other is for taking root scrollable frame's direction. In other words, I don't think that you should touch existing path in the wheel event handlers a lot. Cannot you to just swap deltaX/deltaY with checking new prefs and frame's direction?

Anyway, I don't think that we need hachky behavior with such too big patches since making event handlers complicated causes increasing maintenance cost and just becoming difficult to understand. So, please design the behavior and the patches simpler as far as possible.

Comment on attachment 8957130 [details] Bug 1358017 - Part 1: Introduces a new feature: smart wheel scrolling(only as per the current-target's writing-mode) https://reviewboard.mozilla.org/r/226066/#review232652 > So, I think that you should add new two prefs. Instead of adding new values. One is for making deltaX/deltaY treated as inline/block flow direction. The other is for taking root scrollable frame's direction. > > In other words, I don't think that you should touch existing path in the wheel event handlers a lot. Cannot you to just swap deltaX/deltaY with checking new prefs and frame's direction? Thank you for the review. But I have two questions about this. 1. About "treat x-axis as inline flow direction, y-axis as block flow direction": The phrase is mistaken. Actually both a vertical wheel and a horizontal wheel can be treated as either a scroll in the vertical direction or in the horizontal direction. In other words, it's irrelevant to the block-or-inline thing. So I think you meant "If the scrollable target has only one scrollable direction, then any single wheel scroll is treated as a scroll in that direction". 2. If we add new prefs instead of adding new values, there'll be a ambiguity we need to disambiguate. If the action is 1(Scrolling content), then the meanings of the new two prefs are unambiguous, but what if the action is 2(Treat vertical wheel as horizontal scroll)? What does it mean when action 2 is combined with the new prefs? What do we actually want them to do? It's just ambiguous(This was why I decided to add values instead of prefs). If we are going to add new prefs, I suggest the new prefs only take effect when the action is 1, how do you think about it?

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #35) > Anyway, I don't think that we need hachky behavior with such too big patches > since making event handlers complicated causes increasing maintenance cost > and just becoming difficult to understand. So, please design the behavior > and the patches simpler as far as possible. I don't, too. I always try to minimize code changes if possible, If not, I always try to comment code as clear as possible.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #35) > Anyway, I don't think that we need hachky behavior with such too big patches > since making event handlers complicated causes increasing maintenance cost > and just becoming difficult to understand. So, please design the behavior > and the patches simpler as far as possible. Actually it only involves pretty little code deciding the horizontal edge the smart scroll is towards based on the current target(I.e. S1). In APZ, if |mScrollableRect.x| in struct FrameMetrics is negative, then the horizontal direction starts from right to left: // If the frame is in vertical-RTL writing mode(E.g. "writing-mode: // vertical-rl" in CSS), or if it's in horizontal-RTL writing-mode(E.g. // "writing-mode: horizontal-tb; direction: rtl;" in CSS), then this function // returns true. From the representation perspective, frames whose horizontal // contents start at rightside also cause their horizontal scrollbars, if any, // initially start at rightside. So we can also learn about the initial side // of the horizontal scrollbar for the frame by calling this function. bool IsHorizontalContentRightToLeft() { return mScrollableRect.x < 0; } Just this simple, three lines to get S1 done, not hacky at all. It won't be hacky for the non-APZ part too(GetWritingMode is enough). So I don't think S1 complicated code. Almost all code in Commit 1 is related to neither S1 nor S2, its all just basic code for smart scrolling. S2 even requires more code than S1, but still not too much.

I said not about the code as "hacky". I think that the new behavior is "hacky" especially when there are a lot of scrollable elements nested unexpectedly. I still think that we should be careful which behavior should be enabled in default settings and non-default behavior should be minimized as far as possible. Even if we implement "better" behavior then the other browsers, we shouldn't make users confused by the new behavior. So, I didn't assume that the patch becomes 2 or more patches nor the first patch becomes so big. And let's stop to use "smart" as calling the new behavior because it's unclear. You're trying to use block flow direction to decide scroll direction. So, perhaps there is clearer word to explain what is smarter.

s/stop to use/stop using

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39) > I said not about the code as "hacky". I think that the new behavior is > "hacky" especially when there are a lot of scrollable elements nested > unexpectedly. I still think that we should be careful which behavior should > be enabled in default settings and non-default behavior should be minimized > as far as possible. Even if we implement "better" behavior then the other > browsers, we shouldn't make users confused by the new behavior. > > So, I didn't assume that the patch becomes 2 or more patches nor the first > patch becomes so big. > > And let's stop to use "smart" as calling the new behavior because it's > unclear. You're trying to use block flow direction to decide scroll > direction. So, perhaps there is clearer word to explain what is smarter. I wasn't clear, I meant to assocatie it with your "code maintenance cost" statement when I said "not hacky". The "hacky" in my "not hacky" is nothing assocatied with your "hacky behaviour". I agree with you about simplifing code, but as far as I can tell, without weakening the code readability and reusability, there is no much room for code simplification.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39) > I said not about the code as "hacky". I think that the new behavior is > "hacky" especially when there are a lot of scrollable elements nested > unexpectedly. I still think that we should be careful which behavior should > be enabled in default settings and non-default behavior should be minimized > as far as possible. Even if we implement "better" behavior then the other > browsers, we shouldn't make users confused by the new behavior. > > So, I didn't assume that the patch becomes 2 or more patches nor the first > patch becomes so big. > > And let's stop to use "smart" as calling the new behavior because it's > unclear. You're trying to use block flow direction to decide scroll > direction. So, perhaps there is clearer word to explain what is smarter. Okay. I'll rephrase them in code.

FYI: For minimizing actual changes, you can separate patches for just renaming or refactoring existing code before changing actual behavior. So, patches look bigger if you include both clean up and actual changes.

Commit 1 is definitely the biggest one, because all the basic code is along with it, all the following commits are believed to be much simpler, I schedued to devide the patch into 5 commit because I wanted to make the schedule clearer, not because every commit will be big. So please don't worry about the volume of the whole patch, which won't be much bigger than the first commit.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #43) > FYI: For minimizing actual changes, you can separate patches for just > renaming or refactoring existing code before changing actual behavior. So, > patches look bigger if you include both clean up and actual changes. Now I get what you mean, I mean more small commits, you mean it too. I'll try to devide them into even more samller commits.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39) > And let's stop using "smart" as calling the new behavior because it's > unclear. You're trying to use block flow direction to decide scroll > direction. So, perhaps there is clearer word to explain what is smarter. What about "adaptive scroll", which sounds like "changable"? Since "adaptive" is also widely used nowadays in the web design area to indicate changable sizes or structures. Or "direction-changable scroll" to be even a more specific, though it is somewhat verbose and inconvince for people to talk about it.

(In reply to Zhang Junzhi from comment #46) > > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39) > > You're trying to use block flow direction to decide scroll > > direction. I tend to not use block/inline word to name the scroll, because it's actually irrelevant to the block-or-inline thing. If the scrollable target has only one scrollable direction, then any single wheel scroll is treated as a scroll in that direction. The block-inline-relevant things are actually NOT directions(I.e. vertical/horizontal), they are actually EDGES(I.e. left/right), and only horizontal edges(I.e. left/right) are relevant, vertical ones never are.

typo, tend -> intend

It sounds like the patch is going to go through some cleanup and splitting, which I agree would make it a lot easier to review. In terms of naming, my suggestion is something like "autodir scrolling" which tries to convey the fact that the direction of scrolling is chosen automatically. I'd like to stay away from generic names like "smart" or "adaptive" which aren't specific to the fact that the direction is what is impacted here. (For example, one might think "adaptive" scrolling automatically adapts the scroll speed based on the length of the page, or something).

Comment on attachment 8957130 [details] Bug 1358017 - Part 1: Introduces a new feature: smart wheel scrolling(only as per the current-target's writing-mode) https://reviewboard.mozilla.org/r/226066/#review232782 Clearing review for now, since I'm assuming the patch is going to be split up into smaller pieces. I did do a quick glance over the APZ code changes, and have some comments below. I will do a more detailed review once the patch is split. ::: gfx/layers/apz/src/AsyncPanZoomController.h:568 (Diff revision 1) > > - ParentLayerPoint GetScrollWheelDelta(const ScrollWheelInput& aEvent) const; > + /** > + * Gets the scroll wheel delta's values in device pixels from the original > + * delta's value of a wheel input. > + */ > + ParentLayerPoint GetScrollWheelDeltaInDevicePixels( Please avoid using the term "device pixels" as it is ambiguous. The return type of ParentLayerPoint is already more precise than "device pixels", because it corresponds to a specific coordinate space that is well-defined. See the comments on the various *Pixel structs in layout/base/Units.h for some details. In this case I would prefer to leave the existing method name (GetScrollWheelDelta). ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:31 (Diff revision 1) > +// for APZWheelDeltaSmartizer, WheelDeltaAndMultiplier > +#include "WheelDeltaSmartizer.h" nit: please have the comment on the line with the include, aligned with the other similar comments ::: gfx/layers/apz/src/Axis.h:316 (Diff revision 1) > + enum class Side > + { > + eLeft, > + eRight, > + }; > + bool CanScrollTo(Side aSide) const; Instead of creating a new Side struct I'd prefer if you just used the one already defined at https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/gfx/2d/Types.h#388 While your new Side is more restrictive (in that you define only left/right in AxisX, and top/bottom in AxisY), the CanScrollTo methods still handle default: branches in their switch statements, so the extra restrictiveness doesn't buy you anything. That is, you can use the existing Side class with no real changes to the CanScrollTo method implementations.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #49) > It sounds like the patch is going to go through some cleanup and splitting, > which I agree would make it a lot easier to review. Yes, it is. > In terms of naming, my > suggestion is something like "autodir scrolling" which tries to convey the > fact that the direction of scrolling is chosen automatically. I'd like to > stay away from generic names like "smart" or "adaptive" which aren't > specific to the fact that the direction is what is impacted here. (For > example, one might think "adaptive" scrolling automatically adapts the > scroll speed based on the length of the page, or something). I would go for the name you came up with. Autodir Scrolling sounds just concise enough to me.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50) > ::: gfx/layers/apz/src/AsyncPanZoomController.h:568 > (Diff revision 1) > > > > - ParentLayerPoint GetScrollWheelDelta(const ScrollWheelInput& aEvent) const; > > + /** > > + * Gets the scroll wheel delta's values in device pixels from the original > > + * delta's value of a wheel input. > > + */ > > + ParentLayerPoint GetScrollWheelDeltaInDevicePixels( > > Please avoid using the term "device pixels" as it is ambiguous. The return > type of ParentLayerPoint is already more precise than "device pixels", > because it corresponds to a specific coordinate space that is well-defined. > See the comments on the various *Pixel structs in layout/base/Units.h for > some details. > > In this case I would prefer to leave the existing method name > (GetScrollWheelDelta). > The reason why I prefered a more specific name was that the other delta values(delta for aEvent and WheelDeltaAndMultiplier) were added in the function, so I thought it would confuse people if it didn't have a more specific name, and the function had became longer after adding some lines, people reading it would need to scroll back pages of code to check its type(Assuming they don't have an IDE, or have one without the feature of prompting variable types). However, as you said, |deltaInDevicePixels| is ambiguous. How about |deltaInLayerPixels|? Or do you think it doesn't matter if we still use "delta" as it was?

(In reply to Zhang Junzhi from comment #52) > The reason why I prefered a more specific name was that the other delta > values(delta for aEvent and WheelDeltaAndMultiplier) were added in the > function, so I thought it would confuse people if it didn't have a more > specific name, and the function had became longer after adding some lines, > people reading it would need to scroll back pages of code to check its > type(Assuming they don't have an IDE, or have one without the feature of > prompting variable types). However, as you said, |deltaInDevicePixels| is > ambiguous. How about |deltaInLayerPixels|? Or do you think it doesn't matter > if we still use "delta" as it was? The function is only longer by one line (you removed a blank line, and added two lines). And while you're right that people might have to scroll up a page to check the return type of the function, they would have to do the exact same thing to check the name of the function (which is where you're adding the "InDevicePixels"), so if you're concerned about that then your solution doesn't help all that much (except if you assume people will remember the function name and forget the return type). Regardless, I would prefer leaving it as GetScrollWheelDelta; I don't see anything in those changes that warrants changing the name. Note also that ParentLayerPixels are not the same as LayerPixels, so calling it GetScrollWheelDeltaInLayerPixels would be wrong, since the return value is in ParentLayerPixels.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53) > (In reply to Zhang Junzhi from comment #52) > > The reason why I prefered a more specific name was that the other delta > > values(delta for aEvent and WheelDeltaAndMultiplier) were added in the > > function, so I thought it would confuse people if it didn't have a more > > specific name, and the function had became longer after adding some lines, > > people reading it would need to scroll back pages of code to check its > > type(Assuming they don't have an IDE, or have one without the feature of > > prompting variable types). However, as you said, |deltaInDevicePixels| is > > ambiguous. How about |deltaInLayerPixels|? Or do you think it doesn't matter > > if we still use "delta" as it was? > > The function is only longer by one line (you removed a blank line, and added > two lines). And while you're right that people might have to scroll up a > page to check the return type of the function, they would have to do the > exact same thing to check the name of the function (which is where you're > adding the "InDevicePixels"), so if you're concerned about that then your > solution doesn't help all that much (except if you assume people will > remember the function name and forget the return type). > > Regardless, I would prefer leaving it as GetScrollWheelDelta; I don't see > anything in those changes that warrants changing the name. Note also that > ParentLayerPixels are not the same as LayerPixels, so calling it > GetScrollWheelDeltaInLayerPixels would be wrong, since the return value is > in ParentLayerPixels. Sorry, maybe I was unclear, but you misread what I was talking about, I was talking about the variable name |delta| in OnScrollWheel, which is returned by GetScrollWheelDelta; I was not talking about the function name GetScrollWheelDelta itself, which I had already agreed with you that it doesn't need to be changed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53) > (In reply to Zhang Junzhi from comment #52) > > The reason why I prefered a more specific name was that the other delta > > values(delta for aEvent and WheelDeltaAndMultiplier) were added in the > > function, so I thought it would confuse people if it didn't have a more > > specific name, and the function had became longer after adding some lines, > > people reading it would need to scroll back pages of code to check its > > type(Assuming they don't have an IDE, or have one without the feature of > > prompting variable types). However, as you said, |deltaInDevicePixels| is > > ambiguous. How about |deltaInLayerPixels|? Or do you think it doesn't matter > > if we still use "delta" as it was? > > The function is only longer by one line (you removed a blank line, and added > two lines). And while you're right that people might have to scroll up a > page to check the return type of the function, they would have to do the > exact same thing to check the name of the function (which is where you're > adding the "InDevicePixels"), so if you're concerned about that then your > solution doesn't help all that much (except if you assume people will > remember the function name and forget the return type). > > Regardless, I would prefer leaving it as GetScrollWheelDelta; I don't see > anything in those changes that warrants changing the name. Note also that > ParentLayerPixels are not the same as LayerPixels, so calling it > GetScrollWheelDeltaInLayerPixels would be wrong, since the return value is > in ParentLayerPixels. Oops, it's my fault. By rereading comment 52, I find that I didn't mention I was talking about OnScrollWheel.

Ah. There too I would prefer just calling it |delta| although since it's a local variable I don't care as much. If you do want to rename it I'd prefer |deltaInPLPixels| as that will be more accurate.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > Ah. There too I would prefer just calling it |delta| although since it's a > local variable I don't care as much. If you do want to rename it I'd prefer > |deltaInPLPixels| as that will be more accurate. That variable has a type, which is (hopefully) ParentLayerPoint or similar. What does adding |InPLPixels| to its name accomplish?

(In reply to Botond Ballo [:botond] from comment #57) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > > Ah. There too I would prefer just calling it |delta| although since it's a > > local variable I don't care as much. If you do want to rename it I'd prefer > > |deltaInPLPixels| as that will be more accurate. > > That variable has a type, which is (hopefully) ParentLayerPoint or similar. > What does adding |InPLPixels| to its name accomplish? The patch for this bug is going to extend OnScrollWheel to be longer and define another local delta variables in this function, so I suggested we change that.

I don't have much time these days but have done some work for this, I plan to upload the new patch in a couple of days.

This patch completes S1 and S2 in both APZ and non-APZ, including the Mochitest tests. The only unimplemented part is diagnoal delta detection in GTK+ and Windows. Originally I decided to integrate it in this patch, but the patch soon turned out to be necessarily bigger than my expectation(despite counting out those comment lines). Therefore, I would suggest we file a new bug for implementing diagnoal delta detection; or otherwise the patch will become bloated.

The previously upload attachment 8955540 [details] is a all-round testcase, though it was described in the S1 perspective.

Comment on attachment 8961859 [details] Bug 1358017 - Part 1: Adds some comments, renames some identifiers and refactors some other trivial things. https://reviewboard.mozilla.org/r/230674/#review236574 ::: dom/events/WheelHandlingHelper.h:219 (Diff revision 1) > +enum class WheelDeltaAdjustmentStrategy > +{ > + // This strategy means we're receiving a horizontalized scrolling, we should > + // apply horizontalization strategy for its delta values. > + // Horizontalized scrolling means treating vertical wheel scrolling as > + // horizontal scrolling by adjusting delta values. > + // It's important to keep in mind with the percise concept of horizontalized > + // scrolling: Delta values are *ONLY* going to be adjusted during the process > + // of its default action handling; in views of any programmes other than the > + // default action handler, such as a DOM event listener or a plugin, delta > + // values are never going to be adjusted, they will still retrive original > + // delta values when horizontalization occured for default actions. > + eHorizontalize, > + // TODO A new value for auto-dir scrolling is going to be added while > + // implementing such scrolling. > +}; Why you don't add eNone (or eNothing?) but you use Maybe<> to indicate "treat as is"?

Comment on attachment 8961860 [details] Bug 1358017 - Part 2: Introduces the concept of auto-dir wheel scrolling and adds two new related prefs. https://reviewboard.mozilla.org/r/230676/#review236578 ::: modules/libpref/init/all.js:2857 (Diff revision 1) > +// Auto-dir is a feature which treats any single-wheel scroll as a scroll in the > +// only one scrollable direction if the target has only one scrollable > +// direction. For example, if the user scrolls a vertical wheel inside a target > +// which is horizontally scrollable but vertical unscrollable, then the vertical > +// scroll is converted to a horizontal scroll for that target. > +// Note that auto-dir only takes effect for |mousewheel.*.action|s and > +// |mousewheel.*.action.override_x|s whose values are 1. > +pref("mousewheel.autodir.enabled", true); You use "-" as "Auto-dir", but you don't use "autodir" in the pref name. So, it might be better to use "auto-dir" or "auto_dir" in the pref name for easier to read. Up to you. ::: modules/libpref/init/all.js:2878 (Diff revision 1) > +// If set to true, then consider the root element in the document where the > +// scrolling target is as the honoured target. But note that there's one > +// exception for targets in an HTML document where the <body> element, if any, > +// is in preference of the real root element(I.e. the <html> element) to be > +// actually considered as a root element. > +pref("mousewheel.autodir.honourroot", false); ditto.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #73) > Comment on attachment 8961859 [details] > Bug 1358017 - Part 1: Adds some comments, renames some identifiers and > refactors some other trivial things. > > https://reviewboard.mozilla.org/r/230674/#review236574 > > ::: dom/events/WheelHandlingHelper.h:219 > (Diff revision 1) > > +enum class WheelDeltaAdjustmentStrategy > > +{ > > + // This strategy means we're receiving a horizontalized scrolling, we should > > + // apply horizontalization strategy for its delta values. > > + // Horizontalized scrolling means treating vertical wheel scrolling as > > + // horizontal scrolling by adjusting delta values. > > + // It's important to keep in mind with the percise concept of horizontalized > > + // scrolling: Delta values are *ONLY* going to be adjusted during the process > > + // of its default action handling; in views of any programmes other than the > > + // default action handler, such as a DOM event listener or a plugin, delta > > + // values are never going to be adjusted, they will still retrive original > > + // delta values when horizontalization occured for default actions. > > + eHorizontalize, > > + // TODO A new value for auto-dir scrolling is going to be added while > > + // implementing such scrolling. > > +}; > > Why you don't add eNone (or eNothing?) but you use Maybe<> to indicate > "treat as is"? I use Maybe as a semantic for the enum to not consider the case of no strategy, and intentionally handle the consideration to the implementor. It's up to the implementor as to whether they need to consider the "no strategy" case. So that if one day, it'll be used in a case that it is impossible to have no strategy, the implementor won't need to redefine a new enum. Hence the enum has a higher usability. Since this seems very unlikely to happen, and adding |eNone| will improve performance, yeah, I agree with you, I'll drop Maybe and add |eNone|.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #74) > Comment on attachment 8961860 [details] > Bug 1358017 - Part 2: Introduces the concept of auto-dir wheel scrolling and > adds two new related prefs. > > https://reviewboard.mozilla.org/r/230676/#review236578 > > ::: modules/libpref/init/all.js:2857 > (Diff revision 1) > > +// Auto-dir is a feature which treats any single-wheel scroll as a scroll in the > > +// only one scrollable direction if the target has only one scrollable > > +// direction. For example, if the user scrolls a vertical wheel inside a target > > +// which is horizontally scrollable but vertical unscrollable, then the vertical > > +// scroll is converted to a horizontal scroll for that target. > > +// Note that auto-dir only takes effect for |mousewheel.*.action|s and > > +// |mousewheel.*.action.override_x|s whose values are 1. > > +pref("mousewheel.autodir.enabled", true); > > You use "-" as "Auto-dir", but you don't use "autodir" in the pref name. So, > it might be better to use "auto-dir" or "auto_dir" in the pref name for > easier to read. Up to you. > > ::: modules/libpref/init/all.js:2878 > (Diff revision 1) > > +// If set to true, then consider the root element in the document where the > > +// scrolling target is as the honoured target. But note that there's one > > +// exception for targets in an HTML document where the <body> element, if any, > > +// is in preference of the real root element(I.e. the <html> element) to be > > +// actually considered as a root element. > > +pref("mousewheel.autodir.honourroot", false); > > ditto. smoothscroll and autoscroll is in the style without a hyphen, so autodir here is for keeping the consistent style with smoothscroll and autoscroll. Auto-dir is typically used in a paragraph, so it improves readablity to add a hyphen, while the two new pref items are not in a paragraph so it doesn't affect readablity much. So for the two reasons, I personally prefer the "autodir"s without hyphens for pref.

Comment on attachment 8961859 [details] Bug 1358017 - Part 1: Adds some comments, renames some identifiers and refactors some other trivial things. https://reviewboard.mozilla.org/r/230674/#review236596 Seems reasonable enough. ::: dom/events/EventStateManager.h:649 (Diff revision 1) > * > + * Note that if the default action is ACTION_HORIZONTALIZED_SCROLL and the > + * delta values have been adjusted by WheelDeltaHorizontalizer() before this > + * function is called, this function will swap the X and Y multipliers. By > + * doing this, multipliers will still apply to the delta values they > + * origianlly corresponded to. typo: originally ::: gfx/layers/apz/src/AsyncPanZoomController.h:828 (Diff revision 1) > + // monitor-protected members. That is, although this monitor isn't required to > + // be held before manipulating the class members on their own other than > + // |mFrameMetrics|, |mLastContentPaintMetrics| and |mState|, some of functions The grammar here is a little weird and hard to understand. I'd replace this sentence like so: That is, although this monitor isn't required to be held before manipulating non-protected class members, some functions on those members might indirectly manipulate the protected members; in such cases, the monitor should still be held. ::: gfx/layers/apz/src/AsyncPanZoomController.h:839 (Diff revision 1) > + // Axis::CanScroll(ParentLayerCoord) calls Axis::CanScroll() which calls > + // Axis::GetPageLength() which calls Axis::GetFrameMetrics() which calls > + // AsyncPanZoomController::GetFrameMetrics(), therefore, this monitor should > + // be held before calling the CanScroll function of |mX| and |mY|. These > + // coupled relationships bring us the burden of taking care of when the > + // monitor should be held, so they should be decoupled on our future plans. s/on our future plans/in the future/

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77) > Comment on attachment 8961859 [details] > Bug 1358017 - Part 1: Adds some comments, renames some identifiers and > refactors some other trivial things. > > https://reviewboard.mozilla.org/r/230674/#review236596 > > Seems reasonable enough. > > ::: dom/events/EventStateManager.h:649 > (Diff revision 1) > > * > > + * Note that if the default action is ACTION_HORIZONTALIZED_SCROLL and the > > + * delta values have been adjusted by WheelDeltaHorizontalizer() before this > > + * function is called, this function will swap the X and Y multipliers. By > > + * doing this, multipliers will still apply to the delta values they > > + * origianlly corresponded to. > > typo: originally > > ::: gfx/layers/apz/src/AsyncPanZoomController.h:828 > (Diff revision 1) > > + // monitor-protected members. That is, although this monitor isn't required to > > + // be held before manipulating the class members on their own other than > > + // |mFrameMetrics|, |mLastContentPaintMetrics| and |mState|, some of functions > > The grammar here is a little weird and hard to understand. I'd replace this > sentence like so: > > That is, although this monitor isn't required to be held before manipulating > non-protected class members, some functions on those members might > indirectly manipulate the protected members; in such cases, the monitor > should still be held. > > ::: gfx/layers/apz/src/AsyncPanZoomController.h:839 > (Diff revision 1) > > + // Axis::CanScroll(ParentLayerCoord) calls Axis::CanScroll() which calls > > + // Axis::GetPageLength() which calls Axis::GetFrameMetrics() which calls > > + // AsyncPanZoomController::GetFrameMetrics(), therefore, this monitor should > > + // be held before calling the CanScroll function of |mX| and |mY|. These > > + // coupled relationships bring us the burden of taking care of when the > > + // monitor should be held, so they should be decoupled on our future plans. > > s/on our future plans/in the future/ Okay, all are fixed.

Comment on attachment 8961860 [details] Bug 1358017 - Part 2: Introduces the concept of auto-dir wheel scrolling and adds two new related prefs. https://reviewboard.mozilla.org/r/230676/#review236702 ::: commit-message-deb18:3 (Diff revision 2) > +This commit only introduces the concept and the funtionality of retrieving the > +values from the two new related prefs. Still no actual funtionality change is typo: functionality (in two places) ::: dom/events/WheelHandlingHelper.h:242 (Diff revision 2) > + // the only one scrollable direction if the target has only one scrollable > + // direction. For example, if the user scrolls a vertical wheel inside a > + // target which is horizontally scrollable but vertical unscrollable, then the > + // vertical scroll is converted to a horizontal scroll for that target. > + // So why do we need two different strategies for auto-dir scrolling? That's > + // becasue when a wheel scroll is converted due to auto-dir, there is one typo: because ::: dom/events/WheelHandlingHelper.h:245 (Diff revision 2) > + // vertical scroll is converted to a horizontal scroll for that target. > + // So why do we need two different strategies for auto-dir scrolling? That's > + // becasue when a wheel scroll is converted due to auto-dir, there is one > + // thing called "honoured target" which decides which side the converted > + // scroll goes towards. If the content of the honoured target horizontally > + // starts from right to left, then an upward scroll maps to a rightward scroll s/starts from right to left/is RTL content/ ::: dom/events/WheelHandlingHelper.h:250 (Diff revision 2) > + // |eAutoDirWithRootHonour| considers the root element in the document where > + // the scrolling target is as the honoured target. But note that there's one Slightly easier to understand if you reword this like so: |eAutoDirWithRootHonour| takes the root element of the document with the scrolling element, and considers that as the honoured target. ::: dom/events/WheelHandlingHelper.h:252 (Diff revision 2) > + // exception for targets in an HTML document where the <body> element, if any, > + // is in preference of the real root element(I.e. the <html> element) to be > + // actually considered as a root element. This exception isn't clear to me - do you mean that in some cases you use the <body> element instead of the <html> as the root element? Which cases are those? If these are the same cases as when document.scrollingElement === document.body, then maybe it would better to just reword the previous sentence to refer to document.scrollingElement instead of the "root" element, and omit this sentence talking about the exception. ::: modules/libpref/init/all.js:2873 (Diff revision 2) > +// If set to true, then consider the root element in the document where the > +// scrolling target is as the honoured target. But note that there's one > +// exception for targets in an HTML document where the <body> element, if any, > +// is in preference of the real root element(I.e. the <html> element) to be > +// actually considered as a root element. My previous comments apply here as well.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review236710 ::: dom/events/WheelHandlingHelper.h:301 (Diff revision 2) > int32_t mOldLineOrPageDeltaX; > bool mHorizontalized; > }; > > +/** > + * This class is used to adjust the delta values for wheel scrollingwith the typo: space needed in "scrollingwith" ::: dom/events/WheelHandlingHelper.h:303 (Diff revision 2) > }; > > +/** > + * This class is used to adjust the delta values for wheel scrollingwith the > + * auto-dir functionality. > + * A tranditional wheel scroll only allows the user use the wheel in the same typo: traditional ::: dom/events/WheelHandlingHelper.h:306 (Diff revision 2) > + * This class is used to adjust the delta values for wheel scrollingwith the > + * auto-dir functionality. > + * A tranditional wheel scroll only allows the user use the wheel in the same > + * scrollable direction as that of the scrolling target to scroll the target, > + * whereas an auto-dir scroll lets the user use any wheel(either a vertical > + * wheel or a horizontal tile wheel) to scroll a frame which is scrollable in is "tile" the correct word here? I'm not sure what that means, did you mean "horizontal mouse wheel"? ::: dom/events/WheelHandlingHelper.h:341 (Diff revision 2) > + /** > + * Adjusts the values of the delta values for auto-dir scrolling when > + * ShouldBeAdjusted() returns true. If you call it when ShouldBeAdjusted() > + * returns false, this function will simply do nothing. > + */ > + void adjust(); nit: adjust() should be Adjust() (uppercase A). Update the comments in this file accordingly as well ::: dom/events/WheelHandlingHelper.cpp:689 (Diff revision 2) > + : CanScrollRightwards(); > + } > + return mShouldBeAdjusted; > + } > + } > + mShouldBeAdjusted = false; I'd prefer to initialize mShouldBeAdjusted to false in the constructor initializer list, and then you don't need to set it to false here. You can also change the return value at the end of the function to be |return mShouldBeAdjusted;| and drop the two other |return mShouldBeAdjusted;| statements inside the nested if conditions. This will reduce the number of exit points from the function which makes it a little easier to follow. ::: dom/events/WheelHandlingHelper.cpp:708 (Diff revision 2) > +// Ideally, it's not necessary to define bodies for pure virtual functions, > +// However, we want this class to be exported to the APZ component. > +// If we don't define them, it will fail with undefined reference errors at > +// linking stage. I haven't seen the rest of the patches yet, so I don't know what to make of this. But right now it seems like if you want to put the function bodies here, you can just do that, and make the functions virtual instead of pure-virtual (i.e. drop the "= 0" in the .h file). That's a perfectly fine thing to do, having a regular virtual class with function implementations that just return false and then get overridden by a subclass somewhere.

Comment on attachment 8961862 [details] Bug 1358017 - Part 4: Implements the auto-dir scrolling feature(without the "honour root" functionality) in APZ https://reviewboard.mozilla.org/r/230680/#review236718 Some comments below. Patch looks pretty good overall but I'd like to see a new version with the comments addressed. ::: gfx/layers/apz/src/APZInputBridge.cpp:151 (Diff revision 2) > + default: > + isAutoDir = false; > + honoursRoot = false; > + } Instead of turning WheelDeltaAdjustmentStrategy into two bools, why don't we just put the strategy variable in the ScrollWheelInput and pass it to the APZ that way? It seems like less work and a strongly typed enum is better than two random bools. You can add helper functions on ScrollWheelInput like IsAutoDir() to avoid checking the enum against the two possible enum variants at all the use sites. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2141 (Diff revision 2) > + auto deltaX = aEvent.mDeltaX; > + auto deltaY = aEvent.mDeltaY; > + bool isRTL = IsContentOfHonouredTargetRightToLeft(aEvent.mHonoursRoot); > + APZAutoDirWheelDeltaAdjuster adjuster(deltaX, deltaY, > + mX, mY, > + mRecursiveMutex, Instead of passing the mutex here and locking inside each of the methods individually, you can just acquire the lock inside the containing if block and you don't need to bother passing it around. That's actually better since then you know the axis state won't change in between different operations and you'll get more consistent results. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2165 (Diff revision 2) > + // the |aEvent| and caculate the delta values in parent-layer pixels based > + // on the original delta values from |aEvent|. > + deltaInPLPixels = GetScrollWheelDelta(aEvent); > + } > + > ParentLayerPoint delta = GetScrollWheelDelta(aEvent); It doesn't look like you use |delta| in the rest of this function except for the APZC_LOG. So you should drop this |delta| and rename the |deltaInPLPixels| to just be |delta|. Since there's only one delta and the type is already ParentLayerPixels, I think it's clearer to just leave it as |delta|. ::: gfx/layers/apz/src/Axis.cpp:552 (Diff revision 2) > > +bool AxisX::CanScrollTo(Side aSide) const > +{ > + switch (aSide) { > + case eSideLeft: > + return CanScroll(-COORDINATE_EPSILON * 2); Why the * 2? ::: widget/android/nsWindow.cpp:492 (Diff revision 2) > + // XXX Do we need to support auto-dir scrolling > + // for Android widgets with a wheel device? > + // Currently, I just leave it unimplemented. If Eventually we'll probably want to support this, but for now this is fine. ::: widget/cocoa/nsChildView.mm:5040 (Diff revision 2) > + // XXX Do we need to support auto-dir scrolling > + // for iOS widgets with a wheel device? I don't think you need to do anything special here since this wheel event is sent to DispatchAPZWheelInputEvent, which turns this ScrollWheelInput back into a WidgetWheelEvent and then it goes through the regular handling in APZInputBridge, at https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/widget/cocoa/nsChildView.mm#2959-2964 Same for the other block just below this.

Comment on attachment 8961863 [details] Bug 1358017 - Part 5: Implements the "honour root" functionality for the auto-dir scrolling feature in APZ https://reviewboard.mozilla.org/r/230682/#review236800 This looks ok, but I'm curious about whether you know about document.scrollingElement and how it is sometimes the root element and sometimes the body element. It seems to me that you might want to use that instead of the logic you have now - although it's not the same result as what you have now, it might be more correct. I mentioned document.scrollingElement in a previous comment as well, will wait for your thoughts on that. Masayuki might also have thoughts on that.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #88) Oops, lots of typos, thanks for pointing them out. > > is "tile" the correct word here? I'm not sure what that means, did you mean > "horizontal mouse wheel"? > It's a typo again, there's no such thing as a tile wheel. I meant a tilt wheel which scrolls horizontally.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #89) > Comment on attachment 8961862 [details] > It doesn't look like you use |delta| in the rest of this function except for > the APZC_LOG. So you should drop this |delta| and rename the > |deltaInPLPixels| to just be |delta|. Since there's only one delta and the > type is already ParentLayerPixels, I think it's clearer to just leave it as > |delta|. > Good catch. Strange that I actually noticed these lines but somehow forgot to modify them. > ::: gfx/layers/apz/src/Axis.cpp:552 > (Diff revision 2) > > > > +bool AxisX::CanScrollTo(Side aSide) const > > +{ > > + switch (aSide) { > > + case eSideLeft: > > + return CanScroll(-COORDINATE_EPSILON * 2); > > Why the * 2? > A length that can be identified as scrollable needs to be bigger than COORDINATE_EPSILON. If I just wrote COORDINATE_EPSILON without doubling the value, it would always return false, or if I wrote something like COORDINATE_EPSILON + std::numeric_limits<double>::min, or COORDINATE_EPSILON + some other value, COORDINATE_EPSILON would become coupled with the ensurance of not overflowing std::numeric_limits<double>::min or some other value we use, because double is a floating point value. That's the reason for using *2.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #88) > Comment on attachment 8961861 [details] > ::: dom/events/WheelHandlingHelper.cpp:708 > (Diff revision 2) > > +// Ideally, it's not necessary to define bodies for pure virtual functions, > > +// However, we want this class to be exported to the APZ component. > > +// If we don't define them, it will fail with undefined reference errors at > > +// linking stage. > > I haven't seen the rest of the patches yet, so I don't know what to make of > this. But right now it seems like if you want to put the function bodies > here, you can just do that, and make the functions virtual instead of > pure-virtual (i.e. drop the "= 0" in the .h file). That's a perfectly fine > thing to do, having a regular virtual class with function implementations > that just return false and then get overridden by a subclass somewhere. Yeah, I think the reason that you made the above comment is because you hadn't seen the rest of the patches at that time. Because AutoDirWheelDeltaAdjuster is semantically meant to not be used directly, the purpose of adding these bodies for the pure virtual functions is a hacking for passing the external linking error(At least it's the case for G++ and ld, not sure about other C++ compilers and linkers). And defining a body for a pure virtual function is allowed in C++ standards in itself. Do you agree to keep these "= 0"s after reviewing the rest of the patches?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #87) > This exception isn't clear to me - do you mean that in some cases you use > the <body> element instead of the <html> as the root element? Which cases > are those? If these are the same cases as when document.scrollingElement === > document.body, then maybe it would better to just reword the previous > sentence to refer to document.scrollingElement instead of the "root" > element, and omit this sentence talking about the exception. Oh, I was unclear. The meaning of the sentence isn't exactly the same thing as the document.scrollingElement you mentioned. I wanted to mean <body> is *always* the honoured root instead of <html> for an HTML document, not sometimes(which is the case for document.scrollingElement). That's because whether in standards mode or not, some designers tend to write the overall writing mode in <body> instead of writing it in <html>. So, always honouring the writing mode of <body> seems more reasonable to me. If the designer write the writing mode in <html>, the writing-mode of <body> still inherits the writing mode of <html>, so it still does no harm in my solution. I will make the sentence clearer, what about replacing it with "<body> is *always* the honoured root instead of <html> for an HTML document"?

(In reply to Zhang Junzhi from comment #91) > It's a typo again, there's no such thing as a tile wheel. I meant a tilt > wheel which scrolls horizontally. Ah, that makes sense. (In reply to Zhang Junzhi from comment #92) > > Why the * 2? > > > > A length that can be identified as scrollable needs to be bigger than > COORDINATE_EPSILON. Ok, makes sense. (In reply to Zhang Junzhi from comment #93) > Yeah, I think the reason that you made the above comment is because you > hadn't seen the rest of the patches at that time. Because > AutoDirWheelDeltaAdjuster is semantically meant to not be used directly, the > purpose of adding these bodies for the pure virtual functions is a hacking > for passing the external linking error(At least it's the case for G++ and > ld, not sure about other C++ compilers and linkers). And defining a body for > a pure virtual function is allowed in C++ standards in itself. > > Do you agree to keep these "= 0"s after reviewing the rest of the patches? So now I'm surprised that you were getting a linker error at all. I'll download the patches and do a local build to see what's going on here. Having pure virtual functions with no implementation should be fine as long as the actual subclasses that are instantiated implement those functions. (In reply to Zhang Junzhi from comment #94) > Oh, I was unclear. The meaning of the sentence isn't exactly the same thing > as the document.scrollingElement you mentioned. I wanted to mean <body> is > *always* the honoured root instead of <html> for an HTML document, not > sometimes(which is the case for document.scrollingElement). > > That's because whether in standards mode or not, some designers tend to > write the overall writing mode in <body> instead of writing it in <html>. > So, always honouring the writing mode of <body> seems more reasonable to me. > If the designer write the writing mode in <html>, the writing-mode of <body> > still inherits the writing mode of <html>, so it still does no harm in my > solution. Ah, I see, that makes sense. > I will make the sentence clearer, what about replacing it with "<body> is > *always* the honoured root instead of <html> for an HTML document"? How about something like "If set to true, consider the document's <body> as the honoured root. If there is no <body> element, then consider the <html> element instead." ?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #89) > Comment on attachment 8961862 [details] > Instead of turning WheelDeltaAdjustmentStrategy into two bools, why don't we > just put the strategy variable in the ScrollWheelInput and pass it to the > APZ that way? It seems like less work and a strongly typed enum is better > than two random bools. You can add helper functions on ScrollWheelInput like > IsAutoDir() to avoid checking the enum against the two possible enum > variants at all the use sites. > I did it in order to be just more semantically strict. The |eHorizontalize| in the enum is useless for ScrollWheelInput, because the horizontalization is unlike eAutodir*, it's done before the ScrollWheelInput instance has been created, so I turned WheelDeltaAdjustmentStrategy into two bools to just be more semantically strict. However, as you said, directly passing a WheelDeltaAdjustmentStrategy value is a much lesser work. It seems not necessary to use the two bools, so I'll use WheelDeltaAdjustmentStrategy directly.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #95) > (In reply to Zhang Junzhi from comment #91) > > It's a typo again, there's no such thing as a tile wheel. I meant a tilt > > wheel which scrolls horizontally. > > Ah, that makes sense. > > (In reply to Zhang Junzhi from comment #92) > > > Why the * 2? > > > > > > > A length that can be identified as scrollable needs to be bigger than > > COORDINATE_EPSILON. > > Ok, makes sense. > > (In reply to Zhang Junzhi from comment #93) > > Yeah, I think the reason that you made the above comment is because you > > hadn't seen the rest of the patches at that time. Because > > AutoDirWheelDeltaAdjuster is semantically meant to not be used directly, the > > purpose of adding these bodies for the pure virtual functions is a hacking > > for passing the external linking error(At least it's the case for G++ and > > ld, not sure about other C++ compilers and linkers). And defining a body for > > a pure virtual function is allowed in C++ standards in itself. > > > > Do you agree to keep these "= 0"s after reviewing the rest of the patches? > > So now I'm surprised that you were getting a linker error at all. I'll > download the patches and do a local build to see what's going on here. > Having pure virtual functions with no implementation should be fine as long > as the actual subclasses that are instantiated implement those functions. > Yeah, I was surprised that C++ linker complains undefined reference errors. After I added the definitions, the errors went away. > (In reply to Zhang Junzhi from comment #94) > > Oh, I was unclear. The meaning of the sentence isn't exactly the same thing > > as the document.scrollingElement you mentioned. I wanted to mean <body> is > > *always* the honoured root instead of <html> for an HTML document, not > > sometimes(which is the case for document.scrollingElement). > > > > That's because whether in standards mode or not, some designers tend to > > write the overall writing mode in <body> instead of writing it in <html>. > > So, always honouring the writing mode of <body> seems more reasonable to me. > > If the designer write the writing mode in <html>, the writing-mode of <body> > > still inherits the writing mode of <html>, so it still does no harm in my > > solution. > > Ah, I see, that makes sense. > > > I will make the sentence clearer, what about replacing it with "<body> is > > *always* the honoured root instead of <html> for an HTML document"? > > How about something like "If set to true, consider the document's <body> as > the honoured root. If there is no <body> element, then consider the <html> > element instead." ? Good, it's unambiguous and concise.

I pulled your patchset and it has a bunch of compilation failures. A lot of them are in MOZ_ASSERT which only gets built with debug enabled, so you might want to try doing a debug build locally and clean up the errors that show up. A try push to catch build failures on other platforms would also be a good idea.

Doing an opt build with your patchset and removing the pure-virtual implementations worked fine for me. i.e. I couldn't reproduce the linker errors you were seeing. So from my point of view I think the pure-virtual implementations are unnecessary. If you're still seeing the linker errors can you attach the full build output?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #98) > I pulled your patchset and it has a bunch of compilation failures. A lot of > them are in MOZ_ASSERT which only gets built with debug enabled, so you > might want to try doing a debug build locally and clean up the errors that > show up. A try push to catch build failures on other platforms would also be > a good idea. Ok, I'll build a debug-enabled version to catch these failures. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #99) > Doing an opt build with your patchset and removing the pure-virtual > implementations worked fine for me. i.e. I couldn't reproduce the linker > errors you were seeing. So from my point of view I think the pure-virtual > implementations are unnecessary. If you're still seeing the linker errors > can you attach the full build output? I encountered the link errors, and doing a clobber even didn't help me. But now I cannot reprocude the link errors either. I did a rebase before pushing this group of commits. So it seems that this is the issue caused by a temporary issue from the building system at that time.

Here are the new commits, with all the previously mentioned issues fixed, including those MOZ_ASSERT issues.

Comment on attachment 8961859 [details] Bug 1358017 - Part 1: Adds some comments, renames some identifiers and refactors some other trivial things. https://reviewboard.mozilla.org/r/230674/#review237428

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237430 ::: dom/events/WheelHandlingHelper.h:344 (Diff revision 3) > +private: > + /** > + * Called by Adjust() if Adjust() successfully adjusted the delta values. > + */ > + virtual void OnAdjusted() > + { > + } > + > + virtual bool CanScrollAlongXAxis() const = 0; > + virtual bool CanScrollAlongYAxis() const = 0; > + virtual bool CanScrollUpwards() const = 0; > + virtual bool CanScrollDownwards() const = 0; > + virtual bool CanScrollLeftwards() const = 0; > + virtual bool CanScrollRightwards() const = 0; > + > + /** > + * Gets whether the horizontal content starts at rightside. > + * > + * @return If the content is in vertical-RTL writing mode(E.g. "writing-mode: > + * vertical-rl" in CSS), or if it's in horizontal-RTL writing-mode > + * (E.g. "writing-mode: horizontal-tb; direction: rtl;" in CSS), then > + * this function returns true. From the representation perspective, > + * frames whose horizontal contents start at rightside also cause > + * their horizontal scrollbars, if any, initially start at rightside. > + * So we can also learn about the initial side of the horizontal > + * scrollbar for the frame by calling this function. > + */ > + virtual bool IsHorizontalContentRightToLeft() const = 0; Hmm, I don't like to use virtual methods newly as far as possible. But I need to check if we can avoid using those virtual methods reasonably. ::: dom/events/WheelHandlingHelper.cpp:664 (Diff revision 3) > + // 1. There is only one non-zero value between DeltaX and DeltaY. > + // 2. There is only one direction for the target that overflows and is > + // scrollable with wheel. > + // 3. The direction described in Condition 1 is orthogonal to the one > + // described in Condition 2. > + if (0 != mDeltaX) { Using early-return-style must make this simpler. First, quit if (mDeltaX && mDeltaY). Then, check if (mDeltaX) and return from its block. Then, (mDeltaY) case is only one level indentation. ::: dom/events/WheelHandlingHelper.cpp:666 (Diff revision 3) > + // scrollable with wheel. > + // 3. The direction described in Condition 1 is orthogonal to the one > + // described in Condition 2. > + if (0 != mDeltaX) { > + if (0 == mDeltaY) { > + if (!CanScrollAlongXAxis()) { So, if (CanScrollAlongXAxis()), return. ::: dom/events/WheelHandlingHelper.cpp:677 (Diff revision 3) > + : CanScrollDownwards(); > + } > + } > + } > + } else if (0 != mDeltaY) { > + if (!CanScrollAlongYAxis()) { ditto.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237448 ::: dom/events/WheelHandlingHelper.cpp:696 (Diff revision 3) > + std::swap(mDeltaX, mDeltaY); > + if (IsHorizontalContentRightToLeft()) { > + mDeltaX = -mDeltaX; > + mDeltaY = -mDeltaY; > + } Why don't you modify WidgetWheelEvent::mDeltaValuesHorizontalizedForDefaultHandler? Then, you can make AutoDirWheelDeltaRestorer() simpler.

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review237436 I need to check this patch more, though. ::: dom/events/EventStateManager.h:780 (Diff revision 3) > + // Indicates the wheel scroll event being computed does not mean to be an > + // actual wheel scroll event but merely a tentative "event" used for some > + // other purpose, in this case, the "event" should never be considered as > + // auto-dir even if it is specified as auto-dir, so |aDirectionX| and > + // |aDirectionY| also shouldn't be adjusted. > + IS_TENTATIVE = 0x00000020, I think that ComputeScrollTarget() should be renamed (see following my comment) for making it explains it may horizontize the wheel event. Then, this flag should be reversed its meaning and named as MAYBE_HORIZONTALIZE_WHEEL_EVENT or something. ::: dom/events/EventStateManager.h:826 (Diff revision 3) > + // Be aware that this function might adjust the delta values in |aEvent| if it > + // is auto-dir. For information on auto-dir, > + // @see mozilla::WheelDeltaAdjustmentStrategy > nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame, > WidgetWheelEvent* aEvent, > ComputeScrollTargetOptions aOptions); > > nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame, In such case, please rename the methods. E.g., ComputeScrollTargetAndMaybeHorizontalizeWheelEvent(). And perhaps, you should create inline methods to wrap new long name methods without auto-dir as ComputeScrollTarget() (use MOZ_ASSERT to check new flag is not set). Then, it becomes easier to understand each caller whether it may modify wheel event or not. ::: dom/events/EventStateManager.cpp:2529 (Diff revision 3) > + WheelDeltaAdjustmentStrategy strategy = > + GetWheelDeltaAdjustmentStrategy(*aEvent); > + // If this is neither a legacy event nor a tentative event, and the action > + // is auto-dir, then we respect it as auto-dir. > + if (WheelDeltaAdjustmentStrategy::eAutoDir == strategy) { > + isAutoDir = true; > + honoursRoot = false; > + } else if (WheelDeltaAdjustmentStrategy::eAutoDirWithRootHonour > + == strategy) { > + isAutoDir = true; > + honoursRoot = true; > + } Looks like that using switch-case statement avoids long line and temporary variable. ::: dom/events/EventStateManager.cpp:2678 (Diff revision 3) > + ESMAutoDirWheelDeltaAdjuster adjuster(*aEvent, *scrollFrame, honoursRoot); > + if (adjuster.ShouldBeAdjusted()) { > + adjuster.Adjust(); > + canScroll = true; > + } else if (WheelHandlingUtils::CanScrollOn(scrollableFrame, > + aDirectionX, aDirectionY)) { Remove two whitespaces, you don't need to increase the indent level when listing up arguments. ::: dom/events/EventStateManager.cpp:2682 (Diff revision 3) > + } else if (WheelHandlingUtils::CanScrollOn(scrollableFrame, > + aDirectionX, aDirectionY)) { > + canScroll = true; > + } > + } else if (WheelHandlingUtils::CanScrollOn(scrollableFrame, > + aDirectionX, aDirectionY)) { ditto. ::: dom/events/WheelHandlingHelper.h:11 (Diff revision 3) > > #ifndef mozilla_WheelHandlingHelper_h_ > #define mozilla_WheelHandlingHelper_h_ > > #include "mozilla/Attributes.h" > -#include "mozilla/EventForwards.h" > +#include "mozilla/MouseEvents.h" // for WidgetWheelEvent Looks like that if you stop using decltype of WidgetWheelEvent, you don't need to include MouseEvents.h directly here. If so, please stop using it. I don't think that using decltype is better than reducing rebuild time when MouseEvents.h is changed. ::: dom/events/WheelHandlingHelper.h:401 (Diff revision 3) > + * frame in the same document where the target is; > + * If false, the honoured frame is the scroll > + * target. For the concept of an honoured target, > + * @see mozilla::WheelDeltaAdjustmentStrategy > + */ > + explicit ESMAutoDirWheelDeltaAdjuster( Do we really need the explicit keyword here?? ::: dom/events/WheelHandlingHelper.h:421 (Diff revision 3) > + decltype(WidgetWheelEvent::mOverflowDeltaX)& mOverflowDeltaX; > + decltype(WidgetWheelEvent::mOverflowDeltaY)& mOverflowDeltaY; Please use only one whitespace before "m" because when somebody adds new member whose type name is longer than existing members, they needs to adjust existing member declarations too to align start of the member names. ::: dom/events/WheelHandlingHelper.h:442 (Diff revision 3) > + WidgetWheelEvent& mEvent; > + decltype(WidgetWheelEvent::mDeltaX) mOldDeltaX; > + decltype(WidgetWheelEvent::mDeltaY) mOldDeltaY; > + decltype(WidgetWheelEvent::mLineOrPageDeltaX) mOldLineOrPageDeltaX; > + decltype(WidgetWheelEvent::mLineOrPageDeltaY) mOldLineOrPageDeltaY; > + decltype(WidgetWheelEvent::mOverflowDeltaX) mOldOverflowDeltaX; > + decltype(WidgetWheelEvent::mOverflowDeltaY) mOldOverflowDeltaY; ditto. ::: dom/events/WheelHandlingHelper.cpp:732 (Diff revision 3) > + blockDir == WritingMode::BlockDir::eBlockRL || > + blockDir == WritingMode::BlockDir::eBlockTB && > + inlineDir == WritingMode::InlineDir::eInlineRTL; Use () to making this clearer. ::: dom/events/WheelHandlingHelper.cpp:737 (Diff revision 3) > + blockDir == WritingMode::BlockDir::eBlockRL || > + blockDir == WritingMode::BlockDir::eBlockTB && > + inlineDir == WritingMode::InlineDir::eInlineRTL; > +} > + > +void ESMAutoDirWheelDeltaAdjuster::OnAdjusted() Break the line after return value type. ::: dom/events/WheelHandlingHelper.cpp:743 (Diff revision 3) > +{ > + // Adjust() only adjusted basic deltaX and deltaY, which are not enough for > + // ESM, we should continue to adjust line-or-page and overflow values. > + if (mDeltaX) { > + // A vertical scroll was adjusted to be horizontal. > + MOZ_ASSERT(0 == mDeltaY); MOZ_ASSERT(!mDeltaY); ::: dom/events/WheelHandlingHelper.cpp:747 (Diff revision 3) > + mOverflowDeltaX = mOverflowDeltaY; > + mOverflowDeltaY = 0; Use only one whitespace before "=". ::: dom/events/WheelHandlingHelper.cpp:751 (Diff revision 3) > + mLineOrPageDeltaY = 0; > + mOverflowDeltaX = mOverflowDeltaY; > + mOverflowDeltaY = 0; > + } else { > + // A horizontal scroll was adjusted to be vertical. > + MOZ_ASSERT(0 != mDeltaY); MOZ_ASSERT(mDeltaY); ::: dom/events/WheelHandlingHelper.cpp:755 (Diff revision 3) > + mOverflowDeltaY = mOverflowDeltaX; > + mOverflowDeltaX = 0; Use only one whitespace before "=". ::: dom/events/WheelHandlingHelper.cpp:762 (Diff revision 3) > + mOverflowDeltaX = -mOverflowDeltaX; > + mOverflowDeltaY = -mOverflowDeltaY; ditto. And perhaps, |*= -1| is clearer what this tries to do. ::: dom/events/WheelHandlingHelper.cpp:767 (Diff revision 3) > + mOverflowDeltaX = -mOverflowDeltaX; > + mOverflowDeltaY = -mOverflowDeltaY; > + } > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollAlongXAxis() const Break after |bool|. ::: dom/events/WheelHandlingHelper.cpp:773 (Diff revision 3) > +{ > + return mScrollTargetFrame->GetPerceivedScrollingDirections() & > + nsIScrollableFrame::HORIZONTAL; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollAlongYAxis() const ditto. ::: dom/events/WheelHandlingHelper.cpp:779 (Diff revision 3) > +{ > + return mScrollTargetFrame->GetPerceivedScrollingDirections() & > + nsIScrollableFrame::VERTICAL; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollUpwards() const ditto. ::: dom/events/WheelHandlingHelper.cpp:786 (Diff revision 3) > + nsPoint scrollPt = mScrollTargetFrame->GetScrollPosition(); > + nsRect scrollRange = mScrollTargetFrame->GetScrollRange(); > + return static_cast<double>(scrollRange.y) < scrollPt.y; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollDownwards() const ditto. ::: dom/events/WheelHandlingHelper.cpp:793 (Diff revision 3) > + nsPoint scrollPt = mScrollTargetFrame->GetScrollPosition(); > + nsRect scrollRange = mScrollTargetFrame->GetScrollRange(); > + return static_cast<double>(scrollRange.YMost()) > scrollPt.y; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollLeftwards() const ditto. ::: dom/events/WheelHandlingHelper.cpp:800 (Diff revision 3) > + nsPoint scrollPt = mScrollTargetFrame->GetScrollPosition(); > + nsRect scrollRange = mScrollTargetFrame->GetScrollRange(); > + return static_cast<double>(scrollRange.x) < scrollPt.x; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::CanScrollRightwards() const ditto. ::: dom/events/WheelHandlingHelper.cpp:807 (Diff revision 3) > + nsPoint scrollPt = mScrollTargetFrame->GetScrollPosition(); > + nsRect scrollRange = mScrollTargetFrame->GetScrollRange(); > + return static_cast<double>(scrollRange.XMost()) > scrollPt.x; > +} > + > +bool ESMAutoDirWheelDeltaAdjuster::IsHorizontalContentRightToLeft() const ditto. ::: dom/events/WheelHandlingHelper.cpp:818 (Diff revision 3) > + : mEvent (aEvent) > + , mOldDeltaX (aEvent.mDeltaX) > + , mOldDeltaY (aEvent.mDeltaY) > + , mOldLineOrPageDeltaX (aEvent.mLineOrPageDeltaX) > + , mOldLineOrPageDeltaY (aEvent.mLineOrPageDeltaY) > + , mOldOverflowDeltaX (aEvent.mOverflowDeltaX) > + , mOldOverflowDeltaY (aEvent.mOverflowDeltaY) Remove whitespaces before |(|. ::: dom/events/WheelHandlingHelper.cpp:830 (Diff revision 3) > + if (mOldDeltaX == mEvent.mDeltaX || mOldDeltaY == mEvent.mDeltaY) { > + // The delta of the event wasn't adjusted during the lifetime of this > + // |ESMAutoDirWheelDeltaRestorer| instance. No need to restore it. > + return; > + } Perhaps, you should refer WidgetWheelEvent::mDeltaValuesHorizontalizedForDefaultHandler and reset it. ::: dom/events/WheelHandlingHelper.cpp:840 (Diff revision 3) > + if (mOldDeltaX != mEvent.mDeltaX || mOldDeltaY != mEvent.mDeltaY) > + { put |{| to the end of |if|'s line. ::: dom/events/WheelHandlingHelper.cpp:845 (Diff revision 3) > + mEvent.mDeltaX = -mEvent.mDeltaX; > + mEvent.mDeltaY = -mEvent.mDeltaY; Using |*= -1| must be clearer. ::: dom/events/WheelHandlingHelper.cpp:850 (Diff revision 3) > + if (mEvent.mDeltaX) { > + // A horizontal scroll was adjusted to be vertical during the lifetime of > + // this instance. > + MOZ_ASSERT(0 == mEvent.mDeltaY); > + > + // Restore the line-or-page and overflow values to be horizontal. > + if (forRTL) { > + mEvent.mOverflowDeltaX = -mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = -mEvent.mLineOrPageDeltaY; > + } else { > + mEvent.mOverflowDeltaX = mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = mEvent.mLineOrPageDeltaY; > + } > + mEvent.mOverflowDeltaY = mOldOverflowDeltaY; > + mEvent.mLineOrPageDeltaY = mOldLineOrPageDeltaY; > + } else { > + // A vertical scroll was adjusted to be horizontal during the lifetime of > + // this instance. > + MOZ_ASSERT(0 != mEvent.mDeltaY); > + > + // Restore the line-or-page and overflow values to be vertical. > + if (forRTL) { > + mEvent.mOverflowDeltaY = -mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = -mEvent.mLineOrPageDeltaX; > + } else { > + mEvent.mOverflowDeltaY = mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = mEvent.mLineOrPageDeltaX; > + } > + mEvent.mOverflowDeltaX = mOldOverflowDeltaX; > + mEvent.mLineOrPageDeltaX = mOldLineOrPageDeltaX; > + } > +} I wonder, cannot make this simpler? First, use std::swap() for mLineOrPageDelta and mOverflowDelta. Then, change the sign. Like you do for mDelta. ::: dom/events/WheelHandlingHelper.cpp:853 (Diff revision 3) > + } > + > + if (mEvent.mDeltaX) { > + // A horizontal scroll was adjusted to be vertical during the lifetime of > + // this instance. > + MOZ_ASSERT(0 == mEvent.mDeltaY); MOZ_ASSERT(!mEvent.mDetlaY); ::: dom/events/WheelHandlingHelper.cpp:857 (Diff revision 3) > + // this instance. > + MOZ_ASSERT(0 == mEvent.mDeltaY); > + > + // Restore the line-or-page and overflow values to be horizontal. > + if (forRTL) { > + mEvent.mOverflowDeltaX = -mEvent.mOverflowDeltaY; Use only one whitespace before "=". ::: dom/events/WheelHandlingHelper.cpp:860 (Diff revision 3) > + // Restore the line-or-page and overflow values to be horizontal. > + if (forRTL) { > + mEvent.mOverflowDeltaX = -mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = -mEvent.mLineOrPageDeltaY; > + } else { > + mEvent.mOverflowDeltaX = mEvent.mOverflowDeltaY; ditto. ::: dom/events/WheelHandlingHelper.cpp:863 (Diff revision 3) > + mEvent.mLineOrPageDeltaX = -mEvent.mLineOrPageDeltaY; > + } else { > + mEvent.mOverflowDeltaX = mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = mEvent.mLineOrPageDeltaY; > + } > + mEvent.mOverflowDeltaY = mOldOverflowDeltaY; ditto. ::: dom/events/WheelHandlingHelper.cpp:868 (Diff revision 3) > + mEvent.mOverflowDeltaY = mOldOverflowDeltaY; > + mEvent.mLineOrPageDeltaY = mOldLineOrPageDeltaY; > + } else { > + // A vertical scroll was adjusted to be horizontal during the lifetime of > + // this instance. > + MOZ_ASSERT(0 != mEvent.mDeltaY); MOZ_ASSERT(mEvent.mDeltaY); ::: dom/events/WheelHandlingHelper.cpp:872 (Diff revision 3) > + // this instance. > + MOZ_ASSERT(0 != mEvent.mDeltaY); > + > + // Restore the line-or-page and overflow values to be vertical. > + if (forRTL) { > + mEvent.mOverflowDeltaY = -mEvent.mOverflowDeltaX; Use only one whitespace before "=". ::: dom/events/WheelHandlingHelper.cpp:875 (Diff revision 3) > + // Restore the line-or-page and overflow values to be vertical. > + if (forRTL) { > + mEvent.mOverflowDeltaY = -mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = -mEvent.mLineOrPageDeltaX; > + } else { > + mEvent.mOverflowDeltaY = mEvent.mOverflowDeltaX; ditto. ::: dom/events/WheelHandlingHelper.cpp:878 (Diff revision 3) > + mEvent.mLineOrPageDeltaY = -mEvent.mLineOrPageDeltaX; > + } else { > + mEvent.mOverflowDeltaY = mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = mEvent.mLineOrPageDeltaX; > + } > + mEvent.mOverflowDeltaX = mOldOverflowDeltaX; ditto.

We should probably mention this somewhere.

Comment on attachment 8961862 [details] Bug 1358017 - Part 4: Implements the auto-dir scrolling feature(without the "honour root" functionality) in APZ https://reviewboard.mozilla.org/r/230680/#review237450 ::: commit-message-e58f4:5 (Diff revision 3) > +unimplemented in this commit though the |mHonoursRoot| member is added in > +ScrollWheelInput in this commit in advance. Update commit message to go with the fact that mHonoursRoot is not a member in ScrollWheelInput any more. You can just replace the last sentence with "The functionality of mousewheel.autodir.honourroot will be implemented in a future patch." ::: widget/nsGUIEventIPC.h:1343 (Diff revision 3) > + WriteParam(aMsg, > + static_cast<uint8_t>(aParam.mWheelDeltaAdjustmentStrategy)); Although functionally this will work, the preferred approach is to create another ParamTraits for the enum type. You can use either ContiguousEnumSerializer or ContiguousEnumSerializerInclusive to do it easily. There are examples of both in this nsGUIEventIPC.h file that you can copy. If you use the Inclusive variant you'll probably want to use MOZ_DEFINE_ENUM to define the enum, which will insert a kHighestWheelDeltaAdjustmentStrategy field into the enum that you can use as the upper bound on the serializer. This will make it more robust against additional enum variants being added.

Comment on attachment 8961863 [details] Bug 1358017 - Part 5: Implements the "honour root" functionality for the auto-dir scrolling feature in APZ https://reviewboard.mozilla.org/r/230682/#review237454 ::: gfx/layers/FrameMetrics.h:1041 (Diff revision 3) > + // Bug 1358017 for discussion on it) decides which edge the scroll goes > + // towards based either on the writing mode of the current scrolling frame, or > + // on that of the root primary frame. Which one is based on is configurable > + // through pref. If the user has configured to honour the root primary frame, > + // this variable will play its role. > + bool mIsRootPrimaryFramesHorizontalContentRightToLeft:1; I think we should rename this variable to something like "mIsAutoDirRootContentRTL". In particular I want to avoid using the term "primary frame" because that already has a specific meaning in gecko, and it's not quite the same as what you're defining it as here. So it could get confusing. We can use the term "AutoDirRootContent" to refer to the element that honourroot uses (i.e. the <html> or <body> element). I would likewise simplify this comment: "The AutoDirRootContent is the <body> element in an HTML document, or the root scrollframe if there is no body. This member variable indicates whether this element's content in the horizontal direction starts from right to left (e.g. it's true either if "writing-mode: vertical-rl", or "writing-mode: horizontal-tb; direction: rtl" in CSS). When we do auto-dir scrolling (@see mozilla::WheelDeltaAdjustmentStrategy or refer to bug 1358017 for details), setting a pref can make the code use the writing mode of this root element instead of the target scrollframe, and so we need to know if the writing mode is RTL or not."

Comment on attachment 8961865 [details] Bug 1358017 - Part 7: Implements the "honour root" functionality for the auto-dir scrolling feature in non-APZ https://reviewboard.mozilla.org/r/230686/#review237466 ::: dom/events/WheelHandlingHelper.cpp:741 (Diff revision 3) > + // If there is no <body> frame, fall back to the real root frame. > + honouredFrame = aScrollFrame.PresShell()->GetRootScrollFrame(); > + } > + > + if (!honouredFrame) { > + // If there is no root scroll frame(XXX: is it ever possible?), fall back Cannot test it with MOZ_ASSERT and running all mochitests on tryserver?

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237468 I'd like to check new patch again. ::: dom/events/WheelHandlingHelper.h:319 (Diff revision 3) > + * @param aDeltaX DeltaX for a wheel event whose delta values will > + * be adjusted upon calling Adjust() when > + * ShouldBeAdjusted() returns true. > + * @param aDeltaY DeltaY for a wheel event, like DeltaX. > + */ > + explicit AutoDirWheelDeltaAdjuster(double& aDeltaX, Do we really need this explicit keyword? ::: dom/events/WheelHandlingHelper.h:344 (Diff revision 3) > +private: > + /** > + * Called by Adjust() if Adjust() successfully adjusted the delta values. > + */ > + virtual void OnAdjusted() > + { > + } > + > + virtual bool CanScrollAlongXAxis() const = 0; > + virtual bool CanScrollAlongYAxis() const = 0; > + virtual bool CanScrollUpwards() const = 0; > + virtual bool CanScrollDownwards() const = 0; > + virtual bool CanScrollLeftwards() const = 0; > + virtual bool CanScrollRightwards() const = 0; > + > + /** > + * Gets whether the horizontal content starts at rightside. > + * > + * @return If the content is in vertical-RTL writing mode(E.g. "writing-mode: > + * vertical-rl" in CSS), or if it's in horizontal-RTL writing-mode > + * (E.g. "writing-mode: horizontal-tb; direction: rtl;" in CSS), then > + * this function returns true. From the representation perspective, > + * frames whose horizontal contents start at rightside also cause > + * their horizontal scrollbars, if any, initially start at rightside. > + * So we can also learn about the initial side of the horizontal > + * scrollbar for the frame by calling this function. > + */ > + virtual bool IsHorizontalContentRightToLeft() const = 0; I have no idea. Let's use virtual methods. ::: dom/events/WheelHandlingHelper.cpp:698 (Diff revision 3) > + mDeltaX = -mDeltaX; > + mDeltaY = -mDeltaY; Use |*= -1|.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #110) > Comment on attachment 8961859 [details] > Bug 1358017 - Part 1: Adds some comments, renames some identifiers and > refactors some other trivial things. > > https://reviewboard.mozilla.org/r/230674/#review237428 (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #115) > Comment on attachment 8961862 [details] Oh, so many comments, it takes time to read and get them retified. I am going to deal with the issues and reply the comments step by step, roughly in accordance with the patch's part order.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237430 > Using early-return-style must make this simpler. > > First, quit if (mDeltaX && mDeltaY). Then, check if (mDeltaX) and return from its block. Then, (mDeltaY) case is only one level indentation. Yes, I will change it.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237468 > Do we really need this explicit keyword? The explicit here is used to preemptively avoid the accidental value conversions between different numeric type. So if by any chance, the type of delta is changed one day, the explicit will help us find this issue, and let us change the type of the parameter accordingly. Although very unlikely, the explict does no harm. > Use |*= -1|. Okay, will change it.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237506 ::: dom/events/WheelHandlingHelper.h:319 (Diff revision 3) > + * @param aDeltaX DeltaX for a wheel event whose delta values will > + * be adjusted upon calling Adjust() when > + * ShouldBeAdjusted() returns true. > + * @param aDeltaY DeltaY for a wheel event, like DeltaX. > + */ > + explicit AutoDirWheelDeltaAdjuster(double& aDeltaX, The explicit here is used to preemptively avoid the accidental value conversions between different numeric type. So if by any chance, the type of delta is changed one day, the explicit will help us find this issue, and let us change the type of the parameter accordingly. Although very unlikely, the explict does no harm.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review237448 > Why don't you modify WidgetWheelEvent::mDeltaValuesHorizontalizedForDefaultHandler? Then, you can make AutoDirWheelDeltaRestorer() simpler. Processing mDeltaValuesHorizontalizedForDefaultHandler is easy in non-APZ, and will ease the non-APZ code, but it's unfortunately invisible in APZ when we are in the process of dealing with auto-dir adjustment(AsyncPanZoomController::OnScrollWheel), and moreover, the ScrollWheelInput is const, which will make sharing more difficult. So, it's a question of either complicating APZ code or complicating non-APZ code, it wouldn't buy us much if we shared the member with auto-dir scrolling and horizontalized scrolling. So personally, I would like to suggest we just keep my current solution(I.e. use mDeltaValuesHorizontalizedForDefaultHandler exclusively for horizontalized scrolling) unchanged on this issue.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #118) > > Do we really need this explicit keyword? > Just noticed that all the parameters are references here, so yeah, you're right, the explicit is redundant

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #115) > Comment on attachment 8961862 [details] > Bug 1358017 - Part 4: Implements the auto-dir scrolling feature(without the > "honour root" functionality) in APZ > > https://reviewboard.mozilla.org/r/230680/#review237450 > > ::: commit-message-e58f4:5 > (Diff revision 3) > > +unimplemented in this commit though the |mHonoursRoot| member is added in > > +ScrollWheelInput in this commit in advance. > > Update commit message to go with the fact that mHonoursRoot is not a member > in ScrollWheelInput any more. You can just replace the last sentence with > "The functionality of mousewheel.autodir.honourroot will be implemented in a > future patch." > > ::: widget/nsGUIEventIPC.h:1343 > (Diff revision 3) > > + WriteParam(aMsg, > > + static_cast<uint8_t>(aParam.mWheelDeltaAdjustmentStrategy)); > > Although functionally this will work, the preferred approach is to create > another ParamTraits for the enum type. You can use either > ContiguousEnumSerializer or ContiguousEnumSerializerInclusive to do it > easily. There are examples of both in this nsGUIEventIPC.h file that you can > copy. If you use the Inclusive variant you'll probably want to use > MOZ_DEFINE_ENUM to define the enum, which will insert a > kHighestWheelDeltaAdjustmentStrategy field into the enum that you can use as > the upper bound on the serializer. This will make it more robust against > additional enum variants being added. Will resolve the two issues.

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review237436 > I think that ComputeScrollTarget() should be renamed (see following my comment) for making it explains it may horizontize the wheel event. Then, this flag should be reversed its meaning and named as MAYBE_HORIZONTALIZE_WHEEL_EVENT or something. Changed. > In such case, please rename the methods. E.g., ComputeScrollTargetAndMaybeHorizontalizeWheelEvent(). And perhaps, you should create inline methods to wrap new long name methods without auto-dir as ComputeScrollTarget() (use MOZ_ASSERT to check new flag is not set). Then, it becomes easier to understand each caller whether it may modify wheel event or not. Good to point. Or I have an alternative idea: Name the new compute option with COMPUTE_DEFAULT_ACTION_TARGET_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR, then the method name doesn't need to be changed, because the option name is self-explanatory and we don't need to add a new inline method as well. > Looks like that using switch-case statement avoids long line and temporary variable. Changed. > Remove two whitespaces, you don't need to increase the indent level when listing up arguments. Changed. > ditto. Changed. > Looks like that if you stop using decltype of WidgetWheelEvent, you don't need to include MouseEvents.h directly here. If so, please stop using it. I don't think that using decltype is better than reducing rebuild time when MouseEvents.h is changed. Yes, mozilla/EventForwards.h is just used for decltype of WidgetWheelEvent. So it's a question of whether it's worth making a robust type binding in case of the type change at the cost of building time. > Do we really need the explicit keyword here?? You're right, it's not necessary. Drooped. > Please use only one whitespace before "m" because when somebody adds new member whose type name is longer than existing members, they needs to adjust existing member declarations too to align start of the member names. Changed. > ditto. Changed. > Use () to making this clearer. Added. > Break the line after return value type. Changed. > MOZ_ASSERT(!mDeltaY); Although it won't result in a compilation failure if we change MOZ_ASSERT(0 == mDelta*) to MOZ_ASSERT(!mDelta*), it will result in a compilation failure if we change MOZ_ASSERT(0 != mDelta*) to MOZ_ASSERT(mDelta), because MOZ_ASSERT doesn't allow an implicit conversion from a floating-point number to a bool value. So I have to change MOZ_ASSERT(0 != mDelta*) to MOZ_ASSERT(!!mDelta*), which is not very pretty. Or if we just change MOZ_ASSERT(0 == mDelta*) to MOZ_ASSERT(!mDelta) but keep MOZ_ASSERT(0 != mDelta*) unchanged, they will end up with two inconsistent styles. So I think it might be better to drop this issue, and keep all of this kind of MOZ_ASSERTs unchanged. > Use only one whitespace before "=". Changed. > MOZ_ASSERT(mDeltaY); Please refer to the above comment for the reason I dropped this issue. > Use only one whitespace before "=". Changed. > ditto. And perhaps, |*= -1| is clearer what this tries to do. Changed. > Break after |bool|. Changed. > ditto. Changed. > ditto. Changed. > ditto. Changed. > ditto. Changed. > ditto. Changed. > ditto. Changed. > Remove whitespaces before |(|. Changed. > Perhaps, you should refer WidgetWheelEvent::mDeltaValuesHorizontalizedForDefaultHandler and reset it. My personal opinion for not using mDeltaValuesHorizontalizedForDefaultHandler was listed in comment 123 , and by the way, mDeltaValuesHorizontalizedForDefaultHandler is not a proper name because auto-dir not only horizontalizes a vertical scroll, it also verticalizes a horizontal scroll. > put |{| to the end of |if|'s line. Changed. > Using |*= -1| must be clearer. Changed. > I wonder, cannot make this simpler? First, use std::swap() for mLineOrPageDelta and mOverflowDelta. Then, change the sign. Like you do for mDelta. I haven't figured out a way to use swap instead of just assignments, without slowing the code. And since swapping will not simplify the code much(those if&else conditions seem not removable), so I suggest we just keep it unchanged. > MOZ_ASSERT(!mEvent.mDetlaY); Please refer to the above comment for the reason I dropped this issue. > Use only one whitespace before "=". Changed. > ditto. Changed. > ditto. Changed. > MOZ_ASSERT(mEvent.mDeltaY); Please refer to the above comment for the reason I dropped this issue. > Use only one whitespace before "=". Changed. > ditto. Changed. > ditto. Changed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #116) > Comment on attachment 8961863 [details] > Bug 1358017 - Part 5: Implements the "honour root" functionality for the > auto-dir scrolling feature in APZ > > https://reviewboard.mozilla.org/r/230682/#review237454 > > ::: gfx/layers/FrameMetrics.h:1041 > (Diff revision 3) > > + // Bug 1358017 for discussion on it) decides which edge the scroll goes > > + // towards based either on the writing mode of the current scrolling frame, or > > + // on that of the root primary frame. Which one is based on is configurable > > + // through pref. If the user has configured to honour the root primary frame, > > + // this variable will play its role. > > + bool mIsRootPrimaryFramesHorizontalContentRightToLeft:1; > > I think we should rename this variable to something like > "mIsAutoDirRootContentRTL". In particular I want to avoid using the term > "primary frame" because that already has a specific meaning in gecko, and > it's not quite the same as what you're defining it as here. So it could get > confusing. We can use the term "AutoDirRootContent" to refer to the element > that honourroot uses (i.e. the <html> or <body> element). > > I would likewise simplify this comment: > > "The AutoDirRootContent is the <body> element in an HTML document, or the > root scrollframe if there is no body. This member variable indicates whether > this element's content in the horizontal direction starts from right to left > (e.g. it's true either if "writing-mode: vertical-rl", or "writing-mode: > horizontal-tb; direction: rtl" in CSS). > When we do auto-dir scrolling (@see mozilla::WheelDeltaAdjustmentStrategy or > refer to bug 1358017 for details), setting a pref can make the code use the > writing mode of this root element instead of the target scrollframe, and so > we need to know if the writing mode is RTL or not." Replaced.

I read through the code GetRootScrollFrame calls, and finally confirmed that it's possible for a frame to not own a root scroll frame, so I dropped in the sentence "XXX: is it ever possible?" in part 7. I updated part 8 for completion of auto-dir tests.

s/dropped in/dropped

Just for your information... When I load http://www.gtalbot.org/DHTMLSection/DOM3-events-wheel-event.html in Firefox 61.0a1 buildID=20180330220126 and then put mouse cursor inside the bright green squared area and then hold the <Shift> key and roll mouse wheel, I get horizontal scrolling and the event.deltaY display 3 or -3 depending on if I roll the wheel forward or backward. I would expect to see event.deltaX instead to display 3 or -3 and not event.deltaY since scrolling is horizontal. My mouse has only one wheel.

(In reply to Gérard Talbot from comment #140) > Just for your information... > > When I load > > http://www.gtalbot.org/DHTMLSection/DOM3-events-wheel-event.html > > in Firefox 61.0a1 buildID=20180330220126 and then put mouse cursor inside > the bright green squared area and then hold the <Shift> key and roll mouse > wheel, I get horizontal scrolling and the event.deltaY display 3 or -3 > depending on if I roll the wheel forward or backward. I would expect to see > event.deltaX instead to display 3 or -3 and not event.deltaY since scrolling > is horizontal. > > My mouse has only one wheel. This behaviour for a horizontalized scroll was intentionally designed in this way, and auto-dir for this bug is also designed in this way. There are two reasons for designing in this way: The first reason is that it might be better to make web developers cabable of directly getting what the user is actullay inputting instead of the result the browser encapsulated for them. If the solution is outputting non-zero deltaX for a horizontalized scroll, then the developer has no way to learn of whether the user is inputting [a vertical wheel+Shift Key], or [a horizontal wheel+Shift Key]. With the delta kept unchanged for a horizontalized scroll, web developers are cabable of implementing their own logical behaviour for [a vertical wheel+Shift Key] by preventing the default action in the wheel event listener. If they want to know what the direction the page is scrolling, they can use a scroll event instead of a wheel event. The second reason is that Chrome had also implemented the feature in this way before we did.

Added some "default: break;" cases to prevent compilation errors generated by -Werror=switch on the try servers.

Did you run all mochitests on tryserver? When you modify event default handlers, you should run > ./mach try -b d -p linux64 -u mochitests -t none while you're trying to finish writing patches. Then, if you'll see all green, you should run mochitests in all platforms: > ./mach try -b d -p all -u mochitests -t none Note that only a few tests are not run on debug builds. But they're really rare cases. So, running only on debug build is basically enough in usual bug fixes. (The tryserver resource is not so enough, so, please try to save your cost as far as possible.) Anyway, I'll try to review your patches in a couple of days. Sorry for the delay.

Fixed compilation errors on Android. Fixed errors produced by -Werror,-Wlogical-op-parentheses

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #151) > Did you run all mochitests on tryserver? When you modify event default > handlers, you should run > > ./mach try -b d -p linux64 -u mochitests -t none > while you're trying to finish writing patches. Then, if you'll see all > green, you should run mochitests in all platforms: > > ./mach try -b d -p all -u mochitests -t none Thank you for the guide. I am currenting trying to run mochitests for dom/media, and got some issues and fixed it. I'll continue to run try commands you mentioned above. > Note that only a few tests are not run on debug builds. But they're really > rare cases. So, running only on debug build is basically enough in usual bug > fixes. (The tryserver resource is not so enough, so, please try to save your > cost as far as possible.) > > Anyway, I'll try to review your patches in a couple of days. Sorry for the > delay. Thank you for your review time.

Comment on attachment 8964533 [details] Bug 1358017 - Part 9: Updates some existing mochitests which are not meant to test default actions for wheel events but are affected by auto-dir scrolling. https://reviewboard.mozilla.org/r/233258/#review239930 ::: toolkit/content/tests/chrome/test_mousescroll.xul:346 (Diff revision 3) > +function prepareRunningTests() > +{ > + // Before actually running tests, we disable auto-dir scrolling, becasue the > + // horizontal scrolling tests in this file are mostly meant to ensure that the > + // tested controls in the default style should only have one scrollbar and it > + // must always be in the block-flow direction so they are not really meant to > + // test default actions for wheel events, so we simply disabled auto-dir > + // scrolling, which are well tested in > + // dom/events/test/window_wheel_default_action.html. > + SpecialPowers.pushPrefEnv({"set": [["mousewheel.autodir.enabled", false]]}, > + runTests); FYI: If you make it |async function|, you can write the following code as: await SpecialPowers.pushPrefEnv({"set": [[..., ...]]}); runTests(); But up to you. In this case, not using await is absolutely fine.

Comment on attachment 8961861 [details] Bug 1358017 - Part 3: Defines a common interface of the auto-dir scrolling delta adjuster. https://reviewboard.mozilla.org/r/230678/#review239932 ::: dom/events/WheelHandlingHelper.cpp:664 (Diff revisions 3 - 5) > // 1. There is only one non-zero value between DeltaX and DeltaY. > // 2. There is only one direction for the target that overflows and is > // scrollable with wheel. > // 3. The direction described in Condition 1 is orthogonal to the one > // described in Condition 2. > + if ((0 != mDeltaX && 0 != mDeltaY) || (0 == mDeltaX && 0 == mDeltaY)) { nit: How about |if (!mDeltaX == !mDeltaY)|? If it's difficult to read, |if (mDeltaX && mDeltaY) || (!mDeltaX && !mDeltaY)|. ::: dom/events/WheelHandlingHelper.cpp:667 (Diff revisions 3 - 5) > // 3. The direction described in Condition 1 is orthogonal to the one > // described in Condition 2. > + if ((0 != mDeltaX && 0 != mDeltaY) || (0 == mDeltaX && 0 == mDeltaY)) { > + return false; > + } > if (0 != mDeltaX) { nit: |if (mDeltaX)| ::: dom/events/WheelHandlingHelper.cpp:680 (Diff revisions 3 - 5) > - mShouldBeAdjusted = mDeltaX < 0 ? CanScrollUpwards() > + mShouldBeAdjusted = mDeltaX < 0 ? CanScrollUpwards() > - : CanScrollDownwards(); > + : CanScrollDownwards(); > - } > + } > + return mShouldBeAdjusted; > - } > + } > + MOZ_ASSERT(0 != mDeltaY); nit: |MOZ_ASSERT(mDeltaY);|

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #163) > Comment on attachment 8961861 [details] > > nit: |MOZ_ASSERT(mDeltaY);| MOZ_ASSERT(mDeltaY); will result in compilation error, because MOZ_ASSERT doesn't allow implicit conversion from a floating-point value to a bool value.

(In reply to Zhang Junzhi from comment #164) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #163) > > Comment on attachment 8961861 [details] > > > > nit: |MOZ_ASSERT(mDeltaY);| > > MOZ_ASSERT(mDeltaY); will result in compilation error, because MOZ_ASSERT > doesn't allow implicit conversion from a floating-point value to a bool > value. I originally use MOZ_ASSERT(mDelta*); and MOZ_ASSERT(!mDelta*); After that, I changed them to 0 == and 0 !=. Or MOZ_ASSERT(mDelta*) and MOZ_ASSERT(!!mDelta*)? The latter seems not very pretty.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #162) > Comment on attachment 8964533 [details] > Bug 1358017 - Part 9: Updates some existing mochitests which are not meant > to test default actions for wheel events but are affected by auto-dir > scrolling. > > https://reviewboard.mozilla.org/r/233258/#review239930 > > ::: toolkit/content/tests/chrome/test_mousescroll.xul:346 > (Diff revision 3) > > +function prepareRunningTests() > > +{ > > + // Before actually running tests, we disable auto-dir scrolling, becasue the > > + // horizontal scrolling tests in this file are mostly meant to ensure that the > > + // tested controls in the default style should only have one scrollbar and it > > + // must always be in the block-flow direction so they are not really meant to > > + // test default actions for wheel events, so we simply disabled auto-dir > > + // scrolling, which are well tested in > > + // dom/events/test/window_wheel_default_action.html. > > + SpecialPowers.pushPrefEnv({"set": [["mousewheel.autodir.enabled", false]]}, > > + runTests); > > FYI: If you make it |async function|, you can write the following code as: > > await SpecialPowers.pushPrefEnv({"set": [[..., ...]]}); > runTests(); > > But up to you. In this case, not using await is absolutely fine. Ok, I'll change it.

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review239938 ::: dom/events/EventStateManager.cpp:2540 (Diff revisions 3 - 6) > - == strategy) { > + case WheelDeltaAdjustmentStrategy::eAutoDirWithRootHonour: > - isAutoDir = true; > + isAutoDir = true; > - honoursRoot = true; > + honoursRoot = true; > + break; > + default: > + // Prevent compilation errors generated by -Werror=switch nit: We don't need this comment since we already have a lot of empty default case for this purpose. ::: dom/events/EventStateManager.cpp:2544 (Diff revisions 3 - 6) > - // If the scroll is respected as auto-dir, |aOptions| shouldn't include > - // IS_TENTATIVE, and a non-tentative scroll should always provides its > - // real delta values. > - MOZ_ASSERT(!isAutoDir || > + // If the scroll is respected as auto-dir, aDirection* should always be > + // equivalent to the event's delta vlaues, because an auto-dir scroll is an > + // actual scroll, it'll never be a tentative scroll. so it should always > + // provides its real delta values, not tentative values. It seems that currently, there is no definition of "tentative scroll" due to renaming the flags and this comment is difficult to understand after the "because". ::: dom/events/EventStateManager.cpp:6198 (Diff revisions 3 - 6) > } > return WheelDeltaAdjustmentStrategy::eNone; > case WheelPrefs::ACTION_HORIZONTALIZED_SCROLL: > return WheelDeltaAdjustmentStrategy::eHorizontalize; > + default: > + // Prevent compilation errors generated by -Werror=switch Same, this comment is not necessary. ::: dom/events/EventStateManager.h:801 (Diff revision 6) > // When this is specified, the result may be nsPluginFrame. In such case, > // the frame doesn't have nsIScrollableFrame interface. > COMPUTE_DEFAULT_ACTION_TARGET = > (COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN | > INCLUDE_PLUGIN_AS_TARGET), > + COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR = Wow, too long name. Even only this line, you break 80 characters per line rule... How about COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN_WITH_AUTO_DIR? ::: dom/events/EventStateManager.h:804 (Diff revision 6) > (COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN | > INCLUDE_PLUGIN_AS_TARGET), > + COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR = > + (COMPUTE_DEFAULT_ACTION_TARGET_EXCEPT_PLUGIN | > + MAY_BE_ADJUSTED_BY_AUTO_DIR), > + COMPUTE_DEFAULT_ACTION_TARGET_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR = If so, this can be: COMPUTE_DEFAULT_ACTION_TARGET_WITH_AUTO_DIR. ::: dom/events/EventStateManager.h:813 (Diff revision 6) > + COMPUTE_SCROLLABLE_ANCESTOR_ALONG_X_AXIS_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR = > + (COMPUTE_SCROLLABLE_ANCESTOR_ALONG_X_AXIS | MAY_BE_ADJUSTED_BY_AUTO_DIR), > + COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS_AND_MAY_ADJUST_DELTA_FOR_AUTO_DIR = > + (COMPUTE_SCROLLABLE_ANCESTOR_ALONG_Y_AXIS | MAY_BE_ADJUSTED_BY_AUTO_DIR), So, then, those names can be named with "_WITH_AUTO_DIR" post fix. ::: dom/events/EventStateManager.h:832 (Diff revision 6) > + // Be aware that this function might adjust the delta values in |aEvent| if it > + // is auto-dir. For information on auto-dir, > + // @see mozilla::WheelDeltaAdjustmentStrategy > nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame, > WidgetWheelEvent* aEvent, > ComputeScrollTargetOptions aOptions); > > nsIFrame* ComputeScrollTarget(nsIFrame* aTargetFrame, > double aDirectionX, > double aDirectionY, > WidgetWheelEvent* aEvent, Then, you still need to rename those methods to indicate aEvent may be modified. ComputeScrollTargetAndMayAdjustWheelEvent()? Anyway, I don't like methods named as this kind change argument's value even if flag name indicates the argument may be changed since flag may be wrapped with a variable. Such unclear code may introduce new regressions when the code around the caller is changed. ::: dom/events/EventStateManager.cpp:2544 (Diff revision 6) > + // If the scroll is respected as auto-dir, aDirection* should always be > + // equivalent to the event's delta vlaues, because an auto-dir scroll is an > + // actual scroll, it'll never be a tentative scroll. so it should always > + // provides its real delta values, not tentative values. > + MOZ_ASSERT(!isAutoDir || (aDirectionX == aEvent->mDeltaX && > + aDirectionY == aEvent->mDeltaY)); It's difficult to understand what this comment says after "because". aDirectionX and aDirectionY may be different only when called by ScrollbarsForWheel::TemporarilyActivateAllPossibleScrollTargets(). So, this MOZ_ASSERT() must test if MAY_BE_ADJUSTED_BY_AUTO_DIR isn't set in that case. So, perhaps, you should move this MOZ_ASSERT() to the first line in the above |if| block and just tested if both aDirection(X|Y) match aEvent->mDelta(X|Y). Isn't it? If so, you just say, "aDirection* should always be equivalent to the event's delta values if MAY_BE_ADJUSTED_BY_AUTO_DIR is specified.". On the other hand, I don't see where treat these values as original delta values even after adjusting the event's delta values. So, do we really need this assert? ::: dom/events/EventStateManager.cpp:2600 (Diff revision 6) > + // Always check the frame's scrollability in both the two directions for an > + // auto-dir scroll. The reason is auto-dir shouldn't change scroll direction if the scrollable frame is scrollable to both direction? If so, please put this reason into this comment. ::: dom/events/WheelHandlingHelper.h:347 (Diff revision 6) > * ShouldBeAdjusted() returns true. If you call it when ShouldBeAdjusted() > * returns false, this function will simply do nothing. > */ > void Adjust(); > > + using DeltaValueType = double; Do we really need this? I guess that changing the type in WidgetWheelEvent is changed, broken point will be found by compiler. And this alias is NOT referred by anybody outside of this class except ESMAutoDirWheelDeltaRestorer which is in this file. ::: dom/events/WheelHandlingHelper.h:410 (Diff revision 6) > + using LineOrPageDeltaValueType = int32_t; > + using OverflowDeltaValueType = double; Same for those aliases. Those types are not referred by outside of this class except ESMAutoDirWheelDeltaRestorer which is in this file. So, I don't think that this improves the maintenance cost. ::: dom/events/WheelHandlingHelper.cpp:737 (Diff revision 6) > + (blockDir == WritingMode::BlockDir::eBlockTB && > + inlineDir == WritingMode::InlineDir::eInlineRTL); nit: Typically we align the second line connected with || or && of previous line as: (foo && bar) ::: dom/events/WheelHandlingHelper.cpp:748 (Diff revision 6) > +{ > + // Adjust() only adjusted basic deltaX and deltaY, which are not enough for > + // ESM, we should continue to adjust line-or-page and overflow values. > + if (mDeltaX) { > + // A vertical scroll was adjusted to be horizontal. > + MOZ_ASSERT(0 == mDeltaY); nit: !mDeltaY ::: dom/events/WheelHandlingHelper.cpp:756 (Diff revision 6) > + mLineOrPageDeltaY = 0; > + mOverflowDeltaX = mOverflowDeltaY; > + mOverflowDeltaY = 0; > + } else { > + // A horizontal scroll was adjusted to be vertical. > + MOZ_ASSERT(0 != mDeltaY); nit: MOZ_ASSERT(mDeltaY) ::: dom/events/WheelHandlingHelper.cpp:831 (Diff revision 6) > + : mEvent (aEvent) > + , mOldDeltaX (aEvent.mDeltaX) > + , mOldDeltaY (aEvent.mDeltaY) > + , mOldLineOrPageDeltaX (aEvent.mLineOrPageDeltaX) > + , mOldLineOrPageDeltaY (aEvent.mLineOrPageDeltaY) > + , mOldOverflowDeltaX (aEvent.mOverflowDeltaX) > + , mOldOverflowDeltaY (aEvent.mOverflowDeltaY) nit: remove unnecessary whitespaces before |(|. ::: dom/events/WheelHandlingHelper.cpp:865 (Diff revision 6) > + } > + > + if (mEvent.mDeltaX) { > + // A horizontal scroll was adjusted to be vertical during the lifetime of > + // this instance. > + MOZ_ASSERT(0 == mEvent.mDeltaY); nit: !mEvent.mDeltaY ::: dom/events/WheelHandlingHelper.cpp:868 (Diff revision 6) > + if (forRTL) { > + mEvent.mOverflowDeltaX = -mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = -mEvent.mLineOrPageDeltaY; > + } else { > + mEvent.mOverflowDeltaX = mEvent.mOverflowDeltaY; > + mEvent.mLineOrPageDeltaX = mEvent.mLineOrPageDeltaY; > + } How about: mEvent.mOverflowDeltaX = mEvent.mOverflowDeltaY; mEvent.mLineOrPageDeltaX = mEvent.mLineOrPageDeltaY; if (forRTL) { mEvent.mOverflowDeltaX *= -1; mEvent.mLineOrPageDeltaX *= -1; } ? ::: dom/events/WheelHandlingHelper.cpp:880 (Diff revision 6) > + mEvent.mOverflowDeltaY = mOldOverflowDeltaY; > + mEvent.mLineOrPageDeltaY = mOldLineOrPageDeltaY; > + } else { > + // A vertical scroll was adjusted to be horizontal during the lifetime of > + // this instance. > + MOZ_ASSERT(0 != mEvent.mDeltaY); nit: MOZ_ASSERT(mEvent.mDeltaY) ::: dom/events/WheelHandlingHelper.cpp:883 (Diff revision 6) > + if (forRTL) { > + mEvent.mOverflowDeltaY = -mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = -mEvent.mLineOrPageDeltaX; > + } else { > + mEvent.mOverflowDeltaY = mEvent.mOverflowDeltaX; > + mEvent.mLineOrPageDeltaY = mEvent.mLineOrPageDeltaX; > + } Same as X, how about to set them first, then, if forRTL is true, *= -1?

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review237436 > Although it won't result in a compilation failure if we change MOZ_ASSERT(0 == mDelta*) to MOZ_ASSERT(!mDelta*), it will result in a compilation failure if we change MOZ_ASSERT(0 != mDelta*) to MOZ_ASSERT(mDelta), because MOZ_ASSERT doesn't allow an implicit conversion from a floating-point number to a bool value. So I have to change MOZ_ASSERT(0 != mDelta*) to MOZ_ASSERT(!!mDelta*), which is not very pretty. > Or if we just change MOZ_ASSERT(0 == mDelta*) to MOZ_ASSERT(!mDelta) but keep MOZ_ASSERT(0 != mDelta*) unchanged, they will end up with two inconsistent styles. > So I think it might be better to drop this issue, and keep all of this kind of MOZ_ASSERTs unchanged. Ah, sorry, I didn't check this your reply.

Comment on attachment 8961866 [details] Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. https://reviewboard.mozilla.org/r/230688/#review239980 Keep reviewing... But I'm not sure if I can finish review today. ::: dom/events/test/window_wheel_default_action.html:365 (Diff revision 8) > lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false, > expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0, > shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false }, > expected: kScrollDown }, > { description: "Scroll to top by page scroll even if lineOrPageDelta is 0", > - event: { deltaMode: WheelEvent.DOM_DELTA_LINE, > + event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, Wow! Thanks! ::: dom/events/test/window_wheel_default_action.html:1302 (Diff revision 8) > + } > + doNextTest(); > +} > + > +// It will take *freaking* long time(maybe *hours*) to test all the writing mode I assume that you didn't touch above existing test unless there is some simple bugs and you added the following new methods. I guess this odd diff is caused by bug of MozReview or limitation of hg diff. If this is caused by some changes in the above method, please separate this patch to 2 parts, one is changing existing test, and the other is adding new test. ::: dom/events/test/window_wheel_default_action.html:1306 (Diff revision 8) > +// It will take *freaking* long time(maybe *hours*) to test all the writing mode > +// combinations for the scroll target and its root, because there are altogether > +// *one hundred* combinations (10 x 10)! Sure. Perhaps, we should remove some unrealistic cases of kSettings right now (since your new test increasing the time to run each loop). I'll point it in the definition of kSettings. ::: dom/events/test/window_wheel_default_action.html:4276 (Diff revision 8) > { description: "deltaZ is reverted", > deltaMultiplierX: 1.0, deltaMultiplierY: 1.0, deltaMultiplierZ: -1.0 }, We don't need this case since deltaZ is never set to non-zero for now. Please remove this case. ::: dom/events/test/window_wheel_default_action.html:4282 (Diff revision 8) > { description: "deltaZ is 2.0", > deltaMultiplierX: 1.0, deltaMultiplierY: 1.0, deltaMultiplierZ: 2.0 }, So, only testing deltaZ isn't necessary. Please remove this too. ::: dom/events/test/window_wheel_default_action.html:4284 (Diff revision 8) > { description: "deltaX is -2.0", > deltaMultiplierX: -2.0, deltaMultiplierY: 1.0, deltaMultiplierZ: 1.0 }, > { description: "deltaY is -2.0", > deltaMultiplierX: 1.0, deltaMultiplierY: -2.0, deltaMultiplierZ: 1.0 }, > { description: "deltaZ is -2.0", > deltaMultiplierX: 1.0, deltaMultiplierY: 1.0, deltaMultiplierZ: -2.0 }, Negative multipilers and 2.0 multipliers are tested by above. So, these settings must be redundant. Please remove them. ::: dom/events/test/window_wheel_default_action.html:4294 (Diff revision 8) > function doTest() { > setDeltaMultiplierSettings(kSettings[index], function () { > doTestScroll(kSettings[index], function () { > + doTestAutoDirScroll(kSettings[index], {honoursRoot: false}, function () { > + doTestAutoDirScroll(kSettings[index], {honoursRoot: true}, function () { > - doTestHorizontalizedScroll(kSettings[index], function() { > + doTestHorizontalizedScroll(kSettings[index], function() { > - doTestZoom(kSettings[index], function() { > + doTestZoom(kSettings[index], function() { > - if (++index == kSettings.length) { > + if (++index == kSettings.length) { > - setDeltaMultiplierSettings(kSettings[0], function() { > + setDeltaMultiplierSettings(kSettings[0], function() { > - doTestZoomedScroll(function() { > + doTestZoomedScroll(function() { > - doTestWholeScroll(function() { > + doTestWholeScroll(function() { > - doTestActionOverride(function() { > + doTestActionOverride(function() { > - finishTests(); > + finishTests(); > - }); > + }); > - }); > + }); > - }); > + }); > - }); > + }); > - } else { > + } else { > - doTest(); > + doTest(); > - } > + } > - }); > + }); > - }); > + }); > - }); > + }); > - }); > + }); > + }); > + }); Sigh, out of scope of this bug, though, we should rewrite this test file with async function and await.

Comment on attachment 8961866 [details] Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. https://reviewboard.mozilla.org/r/230688/#review239980 > I assume that you didn't touch above existing test unless there is some simple bugs and you added the following new methods. I guess this odd diff is caused by bug of MozReview or limitation of hg diff. > > If this is caused by some changes in the above method, please separate this patch to 2 parts, one is changing existing test, and the other is adding new test. I didn't change the existing tests, except that I corrected two |DOM_DELTA_LINE|s to |DOM_DELTA_PAGE|s. > Sure. Perhaps, we should remove some unrealistic cases of kSettings right now (since your new test increasing the time to run each loop). I'll point it in the definition of kSettings. Agreed, it is time-consuming.

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review239938 > It seems that currently, there is no definition of "tentative scroll" due to renaming the flags and this comment is difficult to understand after the "because". Good catch. > Then, you still need to rename those methods to indicate aEvent may be modified. ComputeScrollTargetAndMayAdjustWheelEvent()? Anyway, I don't like methods named as this kind change argument's value even if flag name indicates the argument may be changed since flag may be wrapped with a variable. Such unclear code may introduce new regressions when the code around the caller is changed. That makes sense. > It's difficult to understand what this comment says after "because". > > aDirectionX and aDirectionY may be different only when called by ScrollbarsForWheel::TemporarilyActivateAllPossibleScrollTargets(). So, this MOZ_ASSERT() must test if MAY_BE_ADJUSTED_BY_AUTO_DIR isn't set in that case. > > So, perhaps, you should move this MOZ_ASSERT() to the first line in the above |if| block and just tested if both aDirection(X|Y) match aEvent->mDelta(X|Y). Isn't it? > > If so, you just say, "aDirection* should always be equivalent to the event's delta values if MAY_BE_ADJUSTED_BY_AUTO_DIR is specified.". On the other hand, I don't see where treat these values as original delta values even after adjusting the event's delta values. So, do we really need this assert? I agree with that: If so, you just say, "aDirection* should always be equivalent to the event's delta values if MAY_BE_ADJUSTED_BY_AUTO_DIR is specified." > The reason is auto-dir shouldn't change scroll direction if the scrollable frame is scrollable to both direction? If so, please put this reason into this comment. Whether it's scrollable in both directions is checked in AutoDirWheelDeltaAdjuster, so this is not for that reason you thought. The reason is that we need to ignore the PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_X_AXIS and the PREFER_ACTUAL_SCROLLABLE_TARGET_ALONG_Y_AXIS options for auto-dir. Maybe I should refine the comment to make it more understandable. > Do we really need this? I guess that changing the type in WidgetWheelEvent is changed, broken point will be found by compiler. And this alias is NOT referred by anybody outside of this class except ESMAutoDirWheelDeltaRestorer which is in this file. Makes sense

Comment on attachment 8961860 [details] Bug 1358017 - Part 2: Introduces the concept of auto-dir wheel scrolling and adds two new related prefs. https://reviewboard.mozilla.org/r/230676/#review240012 ::: modules/libpref/init/all.js:2857 (Diff revision 5) > +// Auto-dir is a feature which treats any single-wheel scroll as a scroll in the > +// only one scrollable direction if the target has only one scrollable > +// direction. For example, if the user scrolls a vertical wheel inside a target > +// which is horizontally scrollable but vertical unscrollable, then the vertical > +// scroll is converted to a horizontal scroll for that target. > +// Note that auto-dir only takes effect for |mousewheel.*.action|s and > +// |mousewheel.*.action.override_x|s whose values are 1. > +pref("mousewheel.autodir.enabled", true); > +// When a wheel scroll is converted due to auto-dir, which side the converted > +// scroll goes towards is decided by one thing called "honoured target". If the > +// content of the honoured target horizontally starts from right to left, then > +// an upward scroll maps to a rightward scroll and a downward scroll maps to a > +// leftward scroll; otherwise, an upward scroll maps to a leftward scroll and a > +// downward scroll maps to a rightward scroll. > +// If this pref is set to false, then consider the scrolling target as the > +// honoured target. > +// If set to true, then consider the root element in the document where the > +// scrolling target is as the honoured target. But note that there's one > +// exception: for targets in an HTML document, the real root element(I.e. the > +// <html> element) is typically not considered as a root element, but the <body> > +// element is typically considered as a root element. If there is no <body> > +// element, then consider the <html> element instead. > +pref("mousewheel.autodir.honourroot", false); Oh, wait. Landing this patch means that auto-dir will ride the train to be released. But I think that we should keep testing this only on Nightly and early Beta. So, could you make "mousewheel.autodir.enabled" to true only in them for now? Currently, number of testers are not so many. Additionally, not so many web sites use writing-mode. So, perhaps, we need to wait reports in early beta builds for a couple of cycles or list up web sites which use writing-mode and test it by ourselves before release.

Comment on attachment 8961866 [details] Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. https://reviewboard.mozilla.org/r/230688/#review240002 I assume that "writingMode" value is set to writing-mode of CSS and "direction" value is set to direction of CSS. Then, I guess that you disable usual cases too. Perhaps, they should be enabled by default. I think that removing some setting from the test makes those tests allowed to run? Otherwise, perhaps, your new test is run with only a couple of settings. ::: dom/events/test/window_wheel_default_action.html:1321 (Diff revision 8) > +// Also note that |isBTT| has nothing to do with the behaviour of auto-dir > +// scrolling, it's just used to set the sign of |origScrollTop|. > +const kWritingModes = [ Could you explain the structure and each member's meaning here? ::: dom/events/test/window_wheel_default_action.html:1332 (Diff revision 8) > + // uncommon > + //{ > + // writingMode: "vertical-rl", > + // direction: "ltr", > + //}, Why uncommon? This is normal direction for Japanese text. Perhaps, this should be tested. ::: dom/events/test/window_wheel_default_action.html:1337 (Diff revision 8) > + // peculiar > + //{ > + // writingMode: "sideways-rl", > + // direction: "ltr", > + //}, Why is this case "perculiar"? This looks a usual case (although not easy to read sideways-* content). https://jsfiddle.net/d_toybox/grgx2vth/ Perhaps, this should be tested. ::: dom/events/test/window_wheel_default_action.html:1352 (Diff revision 8) > + // uncommon > + //{ > + // writingMode: "vertical-lr", > + // direction: "ltr", > + //}, I think that this is usual case if try to render Mongolian text. However, anyway, as far as I've heard, vertical writing of Mongolian text is not so major. ::: dom/events/test/window_wheel_default_action.html:1357 (Diff revision 8) > + // peculiar > + //{ > + // writingMode: "sideways-lr", > + // direction: "ltr", > + //}, Same. This looks a usual case. https://jsfiddle.net/d_toybox/grgx2vth/1/ Perhaps, this should be tested. ::: dom/events/test/window_wheel_default_action.html:1368 (Diff revision 8) > + // peculiar > + //{ > + // writingMode: "vertical-rl", > + // direction: "rtl", > + //}, > + // peculiar > + //{ > + // writingMode: "sideways-rl", > + // direction: "rtl", > + //}, Agree. Since rtl text is not typically used in vertical writing mode. ::: dom/events/test/window_wheel_default_action.html:1384 (Diff revision 8) > + // peculiar > + //{ > + // writingMode: "vertical-lr", > + // direction: "rtl", > + //}, Agree. ::: dom/events/test/window_wheel_default_action.html:1389 (Diff revision 8) > + // peculiar > + //{ > + // writingMode: "sideways-lr", > + // direction: "rtl", > + //}, Agree. If web designers align the text to upper edge, they should use text-align instead of direction. ::: dom/events/test/window_wheel_default_action.html:1404 (Diff revision 8) > +{ > + if (kWritingModes.length < 1) { > + return false; > + } > + let typeIndex = 0; > + while (kWritingModes[typeIndex].styles.length < 1) { nit: !kWritingModes[typeIndex].styles.length

Comment on attachment 8961866 [details] Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. https://reviewboard.mozilla.org/r/230688/#review240024 I need to check kTests, but sorry, I don't have much time today. ::: dom/events/test/window_wheel_default_action.html:1413 (Diff revision 8) > +function getNextWritingModeStyle(curStyle) > +{ IIRC, { of function block should be placed at end of the last argument's line. Didn't you hit ES orange on tryserver? If it's green, either is okay to me but you should use same style for all new functions. ::: dom/events/test/window_wheel_default_action.html:1433 (Diff revision 8) > + // The whole writing mode list is empty, no need to do any test for auto-dir > + // scrolling. Go ahead with the subsequent tests. > + SimpleTest.executeSoon(aCallback); > + return; Don't we need to put error in this case? ::: dom/events/test/window_wheel_default_action.html:1452 (Diff revision 8) > + const styleTypeForRoot = kWritingModes[aStyleForRoot.typeIndex]; > + const styleTypeForTarget = kWritingModes[aStyleForTarget.typeIndex]; > + > + const styleForRoot = styleTypeForRoot.styles[aStyleForRoot.styleIndex]; > + const styleForTarget = styleTypeForTarget.styles[aStyleForTarget.styleIndex]; > + > + const isRootRTL = styleTypeForRoot.isRTL; > + const isTargetRTL = styleTypeForTarget.isRTL; > + // Just used to set the sign of |origScrollTop|, not related to the auto-dir > + // behaviour. > + const isTargetBTT = styleTypeForTarget.isBTT; > + > + const oldStyleForRoot = {}; > + const oldStyleForTarget = {}; > + > + const honoursRoot = Boolean(aAutoDirTrait.honoursRoot); > + > + const kNoScroll = 0x00; > + const kScrollUp = 0x01; > + const kScrollDown = 0x02; > + const kScrollLeft = 0x04; > + const kScrollRight = 0x08; > + > + // The four constants indicate the expected result if the scroll direction is > + // adjusted. > + const adjustedForUp = {}; > + const adjustedForDown = {}; > + const adjustedForLeft = {}; > + const adjustedForRight = {}; Please use "k" prefix for const. ::: dom/events/test/window_wheel_default_action.html:1467 (Diff revision 8) > + const isTargetBTT = styleTypeForTarget.isBTT; > + > + const oldStyleForRoot = {}; > + const oldStyleForTarget = {}; > + > + const honoursRoot = Boolean(aAutoDirTrait.honoursRoot); When hornoursRoot is false and both typeIndex are same, cannot we skip the test? Same behavior is tested in hornoursRoot is true. ::: dom/events/test/window_wheel_default_action.html:3261 (Diff revision 8) > + var description; > var currentTestIndex = -1; > - // deltaY should cause horizontal scroll and affected by deltaMultiplierY. > - // So, horizontal scroll amount and direction is affected by deltaMultiplierY. > + var isXReverted = aSettings.deltaMultiplierX < 0; > + var isYReverted = aSettings.deltaMultiplierY < 0; Do you need to keep using var actually here? If let is available, it's better. ::: dom/events/test/window_wheel_default_action.html:3266 (Diff revision 8) > + const origScrollLeft = isTargetRTL ? -1000 : 1000; > + const origScrollTop = isTargetBTT ? -1000 : 1000; Use "k" prefix for const. ::: dom/events/test/window_wheel_default_action.html:3333 (Diff revision 8) > + var scrollUp = !isYReverted ? (currentTest.expected & kScrollUp) : > + (currentTest.expected & kScrollDown); > + var scrollDown = !isYReverted ? (currentTest.expected & kScrollDown) : > + (currentTest.expected & kScrollUp); If let is enough, use it instead of var.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #180) > Oh, wait. Landing this patch means that auto-dir will ride the train to be > released. But I think that we should keep testing this only on Nightly and > early Beta. So, could you make "mousewheel.autodir.enabled" to true only in > them for now? Okay. Will change. (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #181) > > +// Also note that |isBTT| has nothing to do with the behaviour of auto-dir > > +// scrolling, it's just used to set the sign of |origScrollTop|. > > +const kWritingModes = [ > > Could you explain the structure and each member's meaning here? The writing modes are all possible style combinations for both the scrolling target and the root. For example, assume we have two styles in |kWritingModes|: { writingMode: "horizontal-tb", direction: "ltr", }, { writingMode: "horizontal-tb", direction: "rtl", }, It means there are two possible styles for both the scrolling target and the root, and the number of all the possible combinations for the scrolling target and the root is 2 x 2 = 4. If we have three styles, then the number of the tested combinations is 3 x 3 = 9, ..., and so on. > > ::: dom/events/test/window_wheel_default_action.html:1332 > (Diff revision 8) > > + // uncommon > > + //{ > > + // writingMode: "vertical-rl", > > + // direction: "ltr", > > + //}, > > Why uncommon? This is normal direction for Japanese text. Perhaps, this > should be tested. > I originally didn't comment it out, but as I said in the above, this will increase the number of tested combinations from 4 to 9, which means doubling the test time for auto-dir at least. > ::: dom/events/test/window_wheel_default_action.html:1337 > (Diff revision 8) > > + // peculiar > > + //{ > > + // writingMode: "sideways-rl", > > + // direction: "ltr", > > + //}, > > Why is this case "perculiar"? This looks a usual case (although not easy to > read sideways-* content). > https://jsfiddle.net/d_toybox/grgx2vth/ Making it as uncommon is reasonable, I suggest we don't make it as common and uncomment it. I cannot think of a case where sideway text is preferred, why not vertical-rl/lr? If we uncomment it, then the number of tested combinations would become 4 x 4 = 16, which would make the tests much slower. > > Perhaps, this should be tested. > > ::: dom/events/test/window_wheel_default_action.html:1352 > (Diff revision 8) > > + // uncommon > > + //{ > > + // writingMode: "vertical-lr", > > + // direction: "ltr", > > + //}, > > I think that this is usual case if try to render Mongolian text. However, > anyway, as far as I've heard, vertical writing of Mongolian text is not so > major. Nowadays, most mongolians use cyrillicized Mogolian which is written horizontally. > > ::: dom/events/test/window_wheel_default_action.html:1357 > (Diff revision 8) > > + // peculiar > > + //{ > > + // writingMode: "sideways-lr", > > + // direction: "ltr", > > + //}, > > Same. This looks a usual case. > https://jsfiddle.net/d_toybox/grgx2vth/1/ > > Perhaps, this should be tested. I suggest not to comment out too many styles, the reason is the booming test time it would bring. > > ::: dom/events/test/window_wheel_default_action.html:1368 > (Diff revision 8) > > + // peculiar > > + //{ > > + // writingMode: "vertical-rl", > > + // direction: "rtl", > > + //}, > > + // peculiar > > + //{ > > + // writingMode: "sideways-rl", > > + // direction: "rtl", > > + //}, > > Agree. Since rtl text is not typically used in vertical writing mode. > > ::: dom/events/test/window_wheel_default_action.html:1384 > (Diff revision 8) > > + // peculiar > > + //{ > > + // writingMode: "vertical-lr", > > + // direction: "rtl", > > + //}, > > Agree. > > ::: dom/events/test/window_wheel_default_action.html:1389 > (Diff revision 8) > > + // peculiar > > + //{ > > + // writingMode: "sideways-lr", > > + // direction: "rtl", > > + //}, > > Agree. If web designers align the text to upper edge, they should use > text-align instead of direction. > > ::: dom/events/test/window_wheel_default_action.html:1404 > (Diff revision 8) > > +{ > > + if (kWritingModes.length < 1) { > > + return false; > > + } > > + let typeIndex = 0; > > + while (kWritingModes[typeIndex].styles.length < 1) { > > nit: !kWritingModes[typeIndex].styles.length Will change it.

s/I suggest not to comment out too many styles/I suggest not to uncomment too many styles

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #182) > Comment on attachment 8961866 [details] > Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. > > https://reviewboard.mozilla.org/r/230688/#review240024 > > I need to check kTests, but sorry, I don't have much time today. > > ::: dom/events/test/window_wheel_default_action.html:1413 > (Diff revision 8) > > +function getNextWritingModeStyle(curStyle) > > +{ > > IIRC, { of function block should be placed at end of the last argument's > line. Didn't you hit ES orange on tryserver? If it's green, either is okay > to me but you should use same style for all new functions. > It's green, but I'll change the style. > ::: dom/events/test/window_wheel_default_action.html:1433 > (Diff revision 8) > > + // The whole writing mode list is empty, no need to do any test for auto-dir > > + // scrolling. Go ahead with the subsequent tests. > > + SimpleTest.executeSoon(aCallback); > > + return; > > Don't we need to put error in this case? > If someone comments out all the kWritingModes, leaving kWritingModes empty(I.e. the person doesn't want to test auto-dir), then this would happen, so there's no need to trigger an error here. > ::: dom/events/test/window_wheel_default_action.html:1452 > (Diff revision 8) > > + const styleTypeForRoot = kWritingModes[aStyleForRoot.typeIndex]; > > + const styleTypeForTarget = kWritingModes[aStyleForTarget.typeIndex]; > > + > > + const styleForRoot = styleTypeForRoot.styles[aStyleForRoot.styleIndex]; > > + const styleForTarget = styleTypeForTarget.styles[aStyleForTarget.styleIndex]; > > + > > + const isRootRTL = styleTypeForRoot.isRTL; > > + const isTargetRTL = styleTypeForTarget.isRTL; > > + // Just used to set the sign of |origScrollTop|, not related to the auto-dir > > + // behaviour. > > + const isTargetBTT = styleTypeForTarget.isBTT; > > + > > + const oldStyleForRoot = {}; > > + const oldStyleForTarget = {}; > > + > > + const honoursRoot = Boolean(aAutoDirTrait.honoursRoot); > > + > > + const kNoScroll = 0x00; > > + const kScrollUp = 0x01; > > + const kScrollDown = 0x02; > > + const kScrollLeft = 0x04; > > + const kScrollRight = 0x08; > > + > > + // The four constants indicate the expected result if the scroll direction is > > + // adjusted. > > + const adjustedForUp = {}; > > + const adjustedForDown = {}; > > + const adjustedForLeft = {}; > > + const adjustedForRight = {}; > > Please use "k" prefix for const. > > ::: dom/events/test/window_wheel_default_action.html:1467 > (Diff revision 8) > > + const isTargetBTT = styleTypeForTarget.isBTT; > > + > > + const oldStyleForRoot = {}; > > + const oldStyleForTarget = {}; > > + > > + const honoursRoot = Boolean(aAutoDirTrait.honoursRoot); > > When hornoursRoot is false and both typeIndex are same, cannot we skip the > test? Same behavior is tested in hornoursRoot is true. > > ::: dom/events/test/window_wheel_default_action.html:3261 > (Diff revision 8) > > + var description; > > var currentTestIndex = -1; > > - // deltaY should cause horizontal scroll and affected by deltaMultiplierY. > > - // So, horizontal scroll amount and direction is affected by deltaMultiplierY. > > + var isXReverted = aSettings.deltaMultiplierX < 0; > > + var isYReverted = aSettings.deltaMultiplierY < 0; > > Do you need to keep using var actually here? If let is available, it's > better. > > ::: dom/events/test/window_wheel_default_action.html:3266 > (Diff revision 8) > > + const origScrollLeft = isTargetRTL ? -1000 : 1000; > > + const origScrollTop = isTargetBTT ? -1000 : 1000; > > Use "k" prefix for const. > > ::: dom/events/test/window_wheel_default_action.html:3333 > (Diff revision 8) > > + var scrollUp = !isYReverted ? (currentTest.expected & kScrollUp) : > > + (currentTest.expected & kScrollDown); > > + var scrollDown = !isYReverted ? (currentTest.expected & kScrollDown) : > > + (currentTest.expected & kScrollUp); > > If let is enough, use it instead of var. The remaining will all be fixed.

Fixed those mentioned issues. And removed the test cases you mentioned, and also removed lots of unrealistic cases in auto-dir(testing non-zero deltaZ). Now there are three styles in kWritingModes enabled: { writingMode: "horizontal-tb", direction: "ltr", }, { writingMode: "horizontal-tb", direction: "rtl", }, { writingMode: "vertical-rl", direction: "ltr", }, In other words, there are 3 x 3 = 9 combinations(There were 2 x 2 = 4 before). But due to the large number of the removed test cases, the test time of window_wheel_default_action.html is still decreased. It now only needs less than 10 minutes.

Comment on attachment 8961866 [details] Bug 1358017 - Part 8: Adds auto-dir scrolling tests for Mochitest. https://reviewboard.mozilla.org/r/230688/#review241678 ::: dom/events/test/window_wheel_default_action.html:1515 (Diff revision 9) > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:1521 (Diff revision 9) > + gScrollableElement.style.top = "10px"; > + gScrollableElement.style.left = "10px"; nit: Remove the redundant whitespaces before "=". ::: dom/events/test/window_wheel_default_action.html:1856 (Diff revision 9) > + // horizontal wheel scrolls are unadjusted. > + // Reason: Auto-dir adjustment applies to a target if the target overflows > + // in only one direction and the direction is orthogonal to the > + // wheel and deltaZ is zero. > + { description: "auto-dir scroll to " + kAdjustedForDown.desc + > + "(originally bottom) by pixel scroll even if lineOrPageDelta is 0, "+ nit: Insert a whitespace before "+". Please do it for the all following descriptions. ::: dom/events/test/window_wheel_default_action.html:1865 (Diff revision 9) > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:1938 (Diff revision 9) > + // Tests: Test pixel scrolling towards four edges when the target > + // overflows only in the vertical direction. > + // Results: Horizontal wheel scrolls are adjusted to be vertical whereas the > + // vertical wheel scrolls are unadjusted. > + // Reason: Auto-dir adjustment applies to a target if the target overflows > + // in only one direction and the direction is orthogonal to the > + // wheel and deltaZ is zero. It might be this is unnecessary hack. According to the actual websites, web developers probably think that horizontal wheel operation is not useful. Actually, I tried to scroll horizontally a lot with my SurfaceBook, but it's really hard. So, it might be enough to have hack only for vertical wheel operation. On the other hand, if restricting to vertical wheel operation makes implementation not so simpler, I don't think that we should do it. Anyway, testers should test current auto-dir scroll with actual websites for a couple of cycles. ::: dom/events/test/window_wheel_default_action.html:1954 (Diff revision 9) > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:2134 (Diff revision 9) > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:2220 (Diff revision 9) > + { description: "auto-dir scroll to " + kAdjustedForDown.desc + > + "(originally bottom) by page scroll even if lineOrPageDelta is 0, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0, > - lineOrPageDeltaX: 0, lineOrPageDeltaY: 1, isMomentum: false, > + lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaY should be 1 here. Since the following event's lineOrPageDeltaY is also 0. And page scroll is not supported by macOS. Therefore, lineOrPageDelta[XY] is always set to non-zero at first wheel event. ::: dom/events/test/window_wheel_default_action.html:2225 (Diff revision 9) > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:2246 (Diff revision 9) > - event: { deltaMode: WheelEvent.DOM_DELTA_LINE, > + { description: "auto-dir scroll to " + kAdjustedForUp.desc + > + "(originally top) by page scroll even if lineOrPageDelta is 0, "+ > + "no vertical scrollbar", > + event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: 0.0, deltaY: -0.5, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, So, lineOrPageDeltaY should be -1 here. ::: dom/events/test/window_wheel_default_action.html:2256 (Diff revision 9) > + { description: "auto-dir scroll to " + kAdjustedForUp.desc + > + "(originally top) by page scroll when lineOrPageDelta is -1, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: 0.0, deltaY: -0.5, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: -1, isMomentum: false, And lineOrPageDeltaY should be 0 here. ::: dom/events/test/window_wheel_default_action.html:2265 (Diff revision 9) > - { description: "Don't scroll by deltaX (page scroll, lineOrPageDelta is 0)", > + expected: kAdjustedForUp.result }, > + { description: "auto-dir scroll to right by page scroll even if lineOrPageDelta is 0, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: 0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be 1. ::: dom/events/test/window_wheel_default_action.html:2274 (Diff revision 9) > - { description: "Don't scroll by deltaX (page scroll, lineOrPageDelta is 1)", > + expected: kScrollRight }, > + { description: "auto-dir scroll to right by page scroll when lineOrPageDelta is 1, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: 0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: 1, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be 0. ::: dom/events/test/window_wheel_default_action.html:2283 (Diff revision 9) > - { description: "Don't scroll by deltaX (page scroll, lineOrPageDelta is 0)", > + expected: kScrollRight }, > + { description: "auto-dir scroll to left by page scroll even if lineOrPageDelta is 0, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: -0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be -1. ::: dom/events/test/window_wheel_default_action.html:2292 (Diff revision 9) > - { description: "Don't scroll by deltaX (page scroll, lineOrPageDelta is -1)", > + expected: kScrollLeft }, > + { description: "auto-dir scroll to left by page scroll when lineOrPageDelta is -1, "+ > + "no vertical scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > deltaX: -0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: -1, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be 0. ::: dom/events/test/window_wheel_default_action.html:2309 (Diff revision 9) > + // wheel and deltaZ is zero. > + { description: "auto-dir scroll to bottom by page scroll even if lineOrPageDelta is 0, "+ > + "no horizontal scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > - deltaX: 0.5, deltaY: 0.5, deltaZ: 0.0, > + deltaX: 0.0, deltaY: 0.5, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaY should be 1. ::: dom/events/test/window_wheel_default_action.html:2315 (Diff revision 9) > expectedOverflowDeltaX: 0, expectedOverflowDeltaY: 0, > - shiftKey: true, ctrlKey: false, altKey: false, metaKey: false, osKey: false }, > - expected: kScrollRight }, > - { description: "Scroll only to right by diagonal page scroll (to bottom-left)", > + shiftKey: false, ctrlKey: false, altKey: false, metaKey: false, osKey: false }, > + adjusted: false, > + expected: kScrollDown, > + prepare: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:2333 (Diff revision 9) > - { description: "Scroll only to left by diagonal page scroll (to top-left)", > + expected: kScrollDown }, > + { description: "auto-dir scroll to top by page scroll even if lineOrPageDelta is 0, "+ > + "no horizontal scrollbar", > event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > - deltaX: -0.5, deltaY: -0.5, deltaZ: 0.0, > + deltaX: 0.0, deltaY: -0.5, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaY should be 1 here even though the previous wheel events only scrolls half of a page. ::: dom/events/test/window_wheel_default_action.html:2352 (Diff revision 9) > + { description: "auto-dir scroll to " + kAdjustedForRight.desc + > + "(originally right) by page scroll even if lineOrPageDelta is 0, "+ > + "no horizontal scrollbar", > + event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > + deltaX: 0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be 1 here even though the previous wheel events only scrolls half of a page. ::: dom/events/test/window_wheel_default_action.html:2372 (Diff revision 9) > + { description: "auto-dir scroll to " + kAdjustedForLeft.desc + > + "(originally left) by page scroll even if lineOrPageDelta is 0, "+ > + "no horizontal scrollbar", > + event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > + deltaX: -0.5, deltaY: 0.0, deltaZ: 0.0, > lineOrPageDeltaX: 0, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be -1 here even though the previous wheel events only scrolls half of a page. ::: dom/events/test/window_wheel_default_action.html:2382 (Diff revision 9) > + { description: "auto-dir scroll to " + kAdjustedForLeft.desc + > + "(originally left) by page scroll when lineOrPageDelta is -1, "+ > + "no horizontal scrollbar", > + event: { deltaMode: WheelEvent.DOM_DELTA_PAGE, > + deltaX: -0.5, deltaY: 0.0, deltaZ: 0.0, > + lineOrPageDeltaX: -1, lineOrPageDeltaY: 0, isMomentum: false, lineOrPageDeltaX should be 0 here. ::: dom/events/test/window_wheel_default_action.html:2387 (Diff revision 9) > + cleanup: function (cb) > + { nit: Move "{" to the end of the previous line. ::: dom/events/test/window_wheel_default_action.html:2390 (Diff revision 9) > + gScrollableElement.style.top = "auto"; > + gScrollableElement.style.left = "auto"; nit: Remove the redundant whitespaces before "=".

Comment on attachment 8961864 [details] Bug 1358017 - Part 6: Implements the auto-dir scrolling feature(without the "honour root" functionality) in non-APZ https://reviewboard.mozilla.org/r/230684/#review241680 Thank you for such big patches! And sorry for really slow review. ::: dom/events/EventStateManager.cpp:2315 (Diff revision 8) > - ComputeScrollTarget(aTargetFrame, aEvent, > - COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET); > + ComputeScrollTargetAndMayAdjustWheelEvent( > + aTargetFrame, aEvent, COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET); In this case, aEvent won't be modified. So, if you create an inline method in the header like: inline nsIFrame* ComputeScrollTarget(...) { MOZ_ASSERT(!(aOptions & MAY_BE_ADJUSTED_BY_AUTO_DIR), "aEvent may be modified by auto-dir"); return ComputeScrollTargetAndMayAdjustWheelEvent(...); } Then, it's clearer for developers reading here. ::: dom/events/EventStateManager.cpp:3570 (Diff revision 8) > - !ComputeScrollTarget(mCurrentTarget, wheelEvent, > - COMPUTE_DEFAULT_ACTION_TARGET); > + !ComputeScrollTargetAndMayAdjustWheelEvent( > + mCurrentTarget, wheelEvent, COMPUTE_DEFAULT_ACTION_TARGET); Then, you can keep using ComputeScrollTarget() here. ::: dom/events/EventStateManager.cpp:5779 (Diff revision 8) > - aESM->ComputeScrollTarget(aTargetFrame, aEvent, > - COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET); > + aESM->ComputeScrollTargetAndMayAdjustWheelEvent( > + aTargetFrame, aEvent, COMPUTE_LEGACY_MOUSE_SCROLL_EVENT_TARGET); And here. ::: dom/events/WheelHandlingHelper.cpp:534 (Diff revision 8) > - aESM->ComputeScrollTarget(aTargetFrame, dir->deltaX, dir->deltaY, aEvent, > + aESM->ComputeScrollTargetAndMayAdjustWheelEvent(aTargetFrame, > + dir->deltaX, dir->deltaY, aEvent, > EventStateManager::COMPUTE_DEFAULT_ACTION_TARGET)); And here.