1438794 - <input>s in vertical writing mode are vertically unscrollable with wheel, but unexpectedly horizontally scrollable

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[張俊芝(ZHANG Junzhi)]

Attachment: test scroll on single line text controls.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180123231643

Steps to reproduce:

Move the mouse pointer on top of the input, and then scroll the vertical wheel and the horizontal wheel respectively(As of Firefox 58: if your mouse doesn't have a horizontal wheel with it, press <em>Alt + Vertical Wheel</em> to simulate horizontal wheel scrolls, assuming you have not changed the default action of <em>Alt + Vertical Wheel</em> in Firefox). For future details, please check the uploaded testcase.


Actual results:

When the mouse wheel scrolls over vertical inputs, only vertical scrolls should take effect, horizontal scrolls should be disregarded. For future details, please check the uploaded testcase.


Expected results:

Vertical scrolls, which should have taken effect, are disregarded; in contrary, horizontal scrolls, which should have been disregarded, take effect. For future details, please check the uploaded testcase.

Comment 1

[張俊芝(ZHANG Junzhi)]

The testcase includes inputs which listen to passive and non-passive scroll events respectively. It shows that both of the vertical inputs behave in the wrong way.

By navigating the source code, I believe I have figured out how to resolve this bug and would like to have a try to work on the "first good" bug for myself. Hope someone would assign this bug for me.

Comment 2

[Alice0775 White]

Shift+wheel for windows

Comment 3

[張俊芝(ZHANG Junzhi)]

(In reply to Alice0775 White from comment #2)
> Shift+wheel for windows

Oops, it is Shift in Linux also. I mistakenly wrote Alt when I meant Shift

Comment 4

[張俊芝(ZHANG Junzhi)]

How can I be assigned to work on this bug? Would someone help assign this to me? I am sure I have figured out the way to fix it

Comment 5

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

Review commit: https://reviewboard.mozilla.org/r/221140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221140/

Comment 6

[張俊芝(ZHANG Junzhi)]

Hello Masayuki, I guess you could potentially be the right reviewer.
One thing worth mentioning: I removed EventStateManager::CanVerticallyScrollFrameWithWheel and add the intrinsicly derived functionality to WheelHandlingUtils::getDisregardedWheelScrollDirection, in order to move the heavy dependence on EventStateManager.h to light dependence on WheelHandlingUtils.h

Comment 7

[張俊芝(ZHANG Junzhi)]

Oh, I just notieced that the first character of function getDisregardedWheelScrollDirection is in lowercase, I am used to that style personally, but it seems to not adhere to Mozilla coding style. No matter whether the patch gets passed, please give it a r- so that I will repatch it to rename it correctly.

Comment 8

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8951846 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

review ping?

Hello, masayuki. I know you are busy, I am just commenting here to ensure that this bug has been seen by you as I am not sure if my pushing to MozReview had automatically notified you.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8951846 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/221140/#review227138

I need to check your new patch. So, marking this as r- for now.

Additionally, after my review, APZ part should be reviewed by Kartikaya Gupta. You should request review only to me next time too. Then, if I'll give you r+, I'll request additional review to him.

::: commit-message-c4d81:1
(Diff revision 1)
> +Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally. r?masayuki
> +

You should write explanation of this patch at least one paragraph after empty line below the first line.

It should explain why this change is necessary and what this change does. Note that, only the first line of commit message has special meaning as summary. However, the detail of this patch should be wrapped at 80 characters per line.

::: dom/events/EventStateManager.cpp:2599
(Diff revision 1)
> +        if (checkIfScrollableX) {
> +          continue;

Hmm, this may be wrong. If the wheel event tries to scroll diagonally, we should scroll current target but just ignore the deltaX. However, current implementation behaves same as this. So, we should keep current behavior at least in this bug for making easier to check regression point for any regression.

::: dom/events/EventStateManager.cpp:2603
(Diff revision 1)
> +      case WheelHandlingUtils::DWSD_VERT:
> -    if (checkIfScrollableY) {
> +        if (checkIfScrollableY) {
> -      if (!CanVerticallyScrollFrameWithWheel(scrollFrame)) {
> -        continue;
> +          continue;
> -      }
> +        }

ditto.

::: dom/events/EventStateManager.cpp:2610
(Diff revision 1)
>      if (!checkIfScrollableX && !checkIfScrollableY) {
>        return frameToScroll;
>      }

I think that you can move this if block before your new switch statement because your new method uses do_QueryInterface but it's expensive. So, we should avoid unnecessary call of it.

::: dom/events/WheelHandlingHelper.h:76
(Diff revision 1)
> +  // Some direction may be disregarded for some special scrollable frames even if
> +  // it does have "scrollable" attribute in that direction. For example, a single
> +  // text control frame should disregard wheel scroll in blcok-flow direction
> +  enum DisregardedWheelScrollDirection
> +  {
> +    DWSD_NONE, DWSD_VERT, DWSD_HORI,

Please do not use abbreviation for enum items. And please use enum class as far as possible. And use e prefix for each of them.  So, this enum should be:

enum class DisregardedWheelScrollDirection
{
  eNone,
  eVertical,
  eHorizontal,
};

And should be referred as DisregardedWheelScrollDirection::eNone etc. Then, you can move this enum class outside of this class. Then, EventStateManager can refer without |WheelHandlingUtils::| but it's enough safe for preventing to conflict with other variables, constants or classes.

If you think those names are too long, I think that "Scroll" is redundant in this case.

::: dom/events/WheelHandlingHelper.h:79
(Diff revision 1)
> +  enum DisregardedWheelScrollDirection
> +  {
> +    DWSD_NONE, DWSD_VERT, DWSD_HORI,
> +  };
> +  static DisregardedWheelScrollDirection
> +  getDisregardedWheelScrollDirection(const nsIFrame* aFrame);

C++ method should be started with uppercase. So, this should be GetDisregardedWheelScrollDirection().

::: dom/events/WheelHandlingHelper.cpp:86
(Diff revision 1)
>  }
>  
> +/*static*/ WheelHandlingUtils::DisregardedWheelScrollDirection
> +WheelHandlingUtils::getDisregardedWheelScrollDirection(const nsIFrame* aFrame)
> +{
> +  nsIContent* c = aFrame->GetContent();

Don't use too short name even for local variables. Basically, each variable name should explain what it is. E.g., |contentOfFrame| is a good name. However, this method is enough small. So, just |content| is fine because it's easy to find which content was set to this variable.

::: gfx/ipc/GfxMessageUtils.h:147
(Diff revision 1)
>  
>    static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
>    {
>      return (ReadParam(aMsg, aIter, &aResult->x) &&
>              ReadParam(aMsg, aIter, &aResult->y));
> - }
> +  }

Please remove this change from this patch because you didn't touch around this whitespace change.

::: gfx/layers/FrameMetrics.h:843
(Diff revision 1)
>             mHasScrollgrab == aOther.mHasScrollgrab &&
> -           mAllowVerticalScrollWithWheel == aOther.mAllowVerticalScrollWithWheel &&
>             mIsLayersIdRoot == aOther.mIsLayersIdRoot &&
>             mUsesContainerScrolling == aOther.mUsesContainerScrolling &&
>             mForceDisableApz == aOther.mForceDisableApz &&
> +           mDisregardedWheelScrollDirection == aOther.mDisregardedWheelScrollDirection &&

nit: over 80 characters. Please wrap after |==| and start next line with 2 whitespaces (i.e., indent++).

::: gfx/layers/FrameMetrics.h:951
(Diff revision 1)
> +  typedef uint8_t Dwsd;
> +  static const Dwsd DWSD_NONE = 1;
> +  static const Dwsd DWSD_HORI = 2;
> +  static const Dwsd DWSD_VERT = 3;

mozilla/WheelHandlingHelper.h can be included everywhere. So, you don't need to declare same contants here.

::: gfx/layers/FrameMetrics.h:955
(Diff revision 1)
> +  // value to be transportable through IPC channels
> +  typedef uint8_t Dwsd;
> +  static const Dwsd DWSD_NONE = 1;
> +  static const Dwsd DWSD_HORI = 2;
> +  static const Dwsd DWSD_VERT = 3;
> +  Dwsd DisregardedWheelScrollDirection() const {

GetDisregarded...() is better.

(IIRC, { should be next line, but this file uses this style. So, using same style is better. So, let's keep using this style here.)

::: gfx/layers/FrameMetrics.h:1016
(Diff revision 1)
> +  // The disregarded direction means the direction which is disregarded anyway, even if
> +  // the scroll frame overflow in that direction and the direction is specified as

nit: Those lines are too long (over 80 chars).

::: gfx/layers/FrameMetrics.h:1020
(Diff revision 1)
>  
> +  // The disregarded direction means the direction which is disregarded anyway, even if
> +  // the scroll frame overflow in that direction and the direction is specified as
> +  // scrollable. For future details on disregarded wheel scroll direction,
> +  // @see mozilla::WheelHandlingUtils::DisregardedWheelScrollDirection
> +  Dwsd mDisregardedWheelScrollDirection;

Okay, you use this type as a member of class. So, the type should be:

enum class DisregardedWheelScrollDirection : uint8_t
{
...

::: gfx/layers/FrameMetrics.h:1031
(Diff revision 1)
>    //
>    // When adding new fields to ScrollMetadata, the following places should be
>    // updated to include them (as needed):
>    //    ScrollMetadata::operator ==
>    //    AsyncPanZoomController::NotifyLayersUpdated
> -  //    The ParamTraits specialization in GfxMessageUtils.h
> +  //    The ParamTraits specialization in GfxMessageUtils.h and/or LayersMessageUtils.h

nit: Too long line.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2727
(Diff revision 1)
> -                                          forceVerticalOverscroll);
> -    bool xChanged = mX.AdjustDisplacement(displacement.x, adjustedDisplacement.x, overscroll.x);
>  
> +    // ScrollMetadata::Dwsd is merely a size-fixed version of
> +    // mozilla::WheelHandlingUtils::DisregardedWheelScrollDirection.
> +    // @see mozilla::WheelHandlingUtils::getDisregardedWheelScrollDirection to understand what a

Too long here and some below lines if this file is written with 80 chars per line rule. However, looks like not so. Please check if your new line is longer than existing lines.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2729
(Diff revision 1)
>  
> +    // ScrollMetadata::Dwsd is merely a size-fixed version of
> +    // mozilla::WheelHandlingUtils::DisregardedWheelScrollDirection.
> +    // @see mozilla::WheelHandlingUtils::getDisregardedWheelScrollDirection to understand what a
> +    // disregarded scroll direction means
> +    disregardedDirection = mScrollMetadata.DisregardedWheelScrollDirection();

Please declare disregardedDirection here because here is the first use. E.g.,

ScrollMetadata::Dwd disregardedDirection =
  mScrollMetaData.DisregardedWheelScrollDirection();

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2730
(Diff revision 1)
> +    // ScrollMetadata::Dwsd is merely a size-fixed version of
> +    // mozilla::WheelHandlingUtils::DisregardedWheelScrollDirection.
> +    // @see mozilla::WheelHandlingUtils::getDisregardedWheelScrollDirection to understand what a
> +    // disregarded scroll direction means
> +    disregardedDirection = mScrollMetadata.DisregardedWheelScrollDirection();
> +    forcesDisregardedDirectionOverscroll = aOverscrollHandoffState.mScrollSource == ScrollSource::Wheel;

And also this should be declared here too.

::: gfx/layers/apz/src/GenericScrollAnimation.h:45
(Diff revision 1)
> -  bool mForceVerticalOverscroll;
> +  enum DirectionForcedToOverscroll
> +  {
> +    DO_NOT_FORCE_OVERSCROLL,
> +    FORCE_OVERSCROLL_HORI,
> +    FORCE_OVERSCROLL_VERT,
> +  }mDirectionForcedToOverscroll;

Please use enum class and specify its type as uint8_t. And use better name for each item.

And please insert a whitespace after }.

::: gfx/layers/apz/src/GenericScrollAnimation.h:45
(Diff revision 1)
>  
>  protected:
>    AsyncPanZoomController& mApzc;
>    UniquePtr<ScrollAnimationPhysics> mAnimationPhysics;
>    nsPoint mFinalDestination;
> -  bool mForceVerticalOverscroll;
> +  enum DirectionForcedToOverscroll

Please add comment to explain what this enum means.

::: layout/base/nsLayoutUtils.cpp:9359
(Diff revision 1)
>      LayoutDeviceIntSize pageScrollAmountInDevPixels =
>        LayoutDeviceIntSize::FromAppUnitsRounded(pageScrollAmount, presContext->AppUnitsPerDevPixel());
>      metadata.SetPageScrollAmount(pageScrollAmountInDevPixels);
>  
> -    if (!aScrollFrame->GetParent() ||
> -        EventStateManager::CanVerticallyScrollFrameWithWheel(aScrollFrame->GetParent()))
> +    if (aScrollFrame->GetParent()) {
> +      WheelHandlingUtils::DisregardedWheelScrollDirection d1;

d1 is really bad name. Perhaps, just disregardedWheelScrollDirection is better.

::: layout/base/nsLayoutUtils.cpp:9360
(Diff revision 1)
>        LayoutDeviceIntSize::FromAppUnitsRounded(pageScrollAmount, presContext->AppUnitsPerDevPixel());
>      metadata.SetPageScrollAmount(pageScrollAmountInDevPixels);
>  
> -    if (!aScrollFrame->GetParent() ||
> -        EventStateManager::CanVerticallyScrollFrameWithWheel(aScrollFrame->GetParent()))
> -    {
> +    if (aScrollFrame->GetParent()) {
> +      WheelHandlingUtils::DisregardedWheelScrollDirection d1;
> +      ScrollMetadata::Dwsd d2;

Ditto. Perhaps, directionForcedToOverscroll is okay.

::: layout/base/nsLayoutUtils.cpp:9361
(Diff revision 1)
>      metadata.SetPageScrollAmount(pageScrollAmountInDevPixels);
>  
> -    if (!aScrollFrame->GetParent() ||
> -        EventStateManager::CanVerticallyScrollFrameWithWheel(aScrollFrame->GetParent()))
> -    {
> -      metadata.SetAllowVerticalScrollWithWheel(true);
> +    if (aScrollFrame->GetParent()) {
> +      WheelHandlingUtils::DisregardedWheelScrollDirection d1;
> +      ScrollMetadata::Dwsd d2;
> +      d1 = WheelHandlingUtils::getDisregardedWheelScrollDirection(aScrollFrame->GetParent());

You should declare |d1| here. Basically, we should declare each variable when it's necessary.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Zhang Junzhi from comment #8)
> review ping?
> 
> Hello, masayuki. I know you are busy, I am just commenting here to ensure
> that this bug has been seen by you as I am not sure if my pushing to
> MozReview had automatically notified you.

Hey, you requested the review on the last Saturday of my timezone. Please let me enjoy weekend.

Comment 11

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8951846 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/221140/#review227138

> You should write explanation of this patch at least one paragraph after empty line below the first line.
> 
> It should explain why this change is necessary and what this change does. Note that, only the first line of commit message has special meaning as summary. However, the detail of this patch should be wrapped at 80 characters per line.

Okay, The next commit message would be as follow:

Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally. r?masayuki

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

> Hmm, this may be wrong. If the wheel event tries to scroll diagonally, we should scroll current target but just ignore the deltaX. However, current implementation behaves same as this. So, we should keep current behavior at least in this bug for making easier to check regression point for any regression.

Yes, I noticed the issue, and that's just what I had in my mind

> I think that you can move this if block before your new switch statement because your new method uses do_QueryInterface but it's expensive. So, we should avoid unnecessary call of it.

Fixed it.

> C++ method should be started with uppercase. So, this should be GetDisregardedWheelScrollDirection().

Good catch.

> mozilla/WheelHandlingHelper.h can be included everywhere. So, you don't need to declare same contants here.

I considered to use the same enum in WheelHandlingHelper.h to begin with.

But one of the issues is the file inclusion relationship will turn into recursive that way: FrameMetrics.h => WheelHandlingHelper.h => nsIFrame.h => FrameMetrics.h. Because the outer FrameMetrics.h has already defined GFX_FRAMEMETRICS_H, the inner FrameMetrics.h will be compiled simply to nothing, which will eventually cause compilation errors of undeclared types and variables.

The other issue is the paired functions ReadParam and WriteParam in LayersMessageUtils.h are based on template<> struct ParamTraits<...> which are then based on Write and Read in chrome/common/ipc_message_utils.h. So this could require code modifications in a too-deep hell for just using one same enum, so my opinion is use uint8_t instead, which ReadParam and WriteParam already support. I think this is a much cheaper solution.

> GetDisregarded...() is better.
> 
> (IIRC, { should be next line, but this file uses this style. So, using same style is better. So, let's keep using this style here.)

Okay. I intentionally omitted Get to keep the function naming style consistent in that class. Adding Get looks better to me as well.

> Okay, you use this type as a member of class. So, the type should be:
> 
> enum class DisregardedWheelScrollDirection : uint8_t
> {
> ...

The reason why I finally decided to use uint8_t is the same as the second reason why I didn't use the same enum in WheelHandlingHelper.h

> Ditto. Perhaps, directionForcedToOverscroll is okay.

Fine, renamed them. d1 and d2 both stand for disregarded direction, the latter is for meta data use in a fixed size, so it's just a thing of format conversion

Comment 12

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> (In reply to Zhang Junzhi from comment #8)
> > review ping?
> > 
> > Hello, masayuki. I know you are busy, I am just commenting here to ensure
> > that this bug has been seen by you as I am not sure if my pushing to
> > MozReview had automatically notified you.
> 
> Hey, you requested the review on the last Saturday of my timezone. Please
> let me enjoy weekend.

Aha, Maybe I am pretty eager to scratch an itch. :)

Comment 13

[張俊芝(ZHANG Junzhi)]

The changeset is rebased to the most recently-updated in-bound code, and I am recompiling it. I will commit a new review request upon compilation success.

Comment 14

[張俊芝(ZHANG Junzhi)]

> ::: dom/events/EventStateManager.cpp:2610
> (Diff revision 1)
> >      if (!checkIfScrollableX && !checkIfScrollableY) {
> >        return frameToScroll;
> >      }
> 
> I think that you can move this if block before your new switch statement
> because your new method uses do_QueryInterface but it's expensive. So, we
> should avoid unnecessary call of it.
> 

Oh, the call is necessary before this condition check. I just found that when I got a compilation error: the variable frameToScroll is the returned value of do_QueryInterface. So they cannot be reordered. Doesn't need to be fixed.

Comment 15

[張俊芝(ZHANG Junzhi)]

> ::: dom/events/EventStateManager.cpp:2610
> (Diff revision 1)
> >      if (!checkIfScrollableX && !checkIfScrollableY) {
> >        return frameToScroll;
> >      }
> 
> I think that you can move this if block before your new switch statement
> because your new method uses do_QueryInterface but it's expensive. So, we
> should avoid unnecessary call of it.
> 

You are right, just ignore comment#14. They do need to be reordered. :)
You meant do_QueryFrame, and I misread it as do_QueryInterface.

Comment 16

[張俊芝(ZHANG Junzhi)]

> ::: dom/events/EventStateManager.cpp:2610
> (Diff revision 1)
> >      if (!checkIfScrollableX && !checkIfScrollableY) {
> >        return frameToScroll;
> >      }
> 
> I think that you can move this if block before your new switch statement
> because your new method uses do_QueryInterface but it's expensive. So, we
> should avoid unnecessary call of it.
>

Opps, comment#15 was miswritten. one miswriting after one misreading. What is wrong with me?

I meants:
You meant do_QueryInterface, and I misread it as do_QueryFrame.

Comment 17

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

Review commit: https://reviewboard.mozilla.org/r/221444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221444/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8951846 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/221140/#review227138

> I considered to use the same enum in WheelHandlingHelper.h to begin with.
> 
> But one of the issues is the file inclusion relationship will turn into recursive that way: FrameMetrics.h => WheelHandlingHelper.h => nsIFrame.h => FrameMetrics.h. Because the outer FrameMetrics.h has already defined GFX_FRAMEMETRICS_H, the inner FrameMetrics.h will be compiled simply to nothing, which will eventually cause compilation errors of undeclared types and variables.
> 
> The other issue is the paired functions ReadParam and WriteParam in LayersMessageUtils.h are based on template<> struct ParamTraits<...> which are then based on Write and Read in chrome/common/ipc_message_utils.h. So this could require code modifications in a too-deep hell for just using one same enum, so my opinion is use uint8_t instead, which ReadParam and WriteParam already support. I think this is a much cheaper solution.

After you change it to |enum class DisregardedWheelScrollDirection : uint8_t|, you can use forward declaration. Cannot fix it?

Or, perhaps, FrameMetrics.h should have the declaration instead.

Comment 19

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> Comment on attachment 8951846 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> https://reviewboard.mozilla.org/r/221140/#review227138
> 
> > I considered to use the same enum in WheelHandlingHelper.h to begin with.
> > 
> > But one of the issues is the file inclusion relationship will turn into recursive that way: FrameMetrics.h => WheelHandlingHelper.h => nsIFrame.h => FrameMetrics.h. Because the outer FrameMetrics.h has already defined GFX_FRAMEMETRICS_H, the inner FrameMetrics.h will be compiled simply to nothing, which will eventually cause compilation errors of undeclared types and variables.
> > 
> > The other issue is the paired functions ReadParam and WriteParam in LayersMessageUtils.h are based on template<> struct ParamTraits<...> which are then based on Write and Read in chrome/common/ipc_message_utils.h. So this could require code modifications in a too-deep hell for just using one same enum, so my opinion is use uint8_t instead, which ReadParam and WriteParam already support. I think this is a much cheaper solution.
> 
> After you change it to |enum class DisregardedWheelScrollDirection :
> uint8_t|, you can use forward declaration. Cannot fix it?
> 
> Or, perhaps, FrameMetrics.h should have the declaration instead.

Forward declaration requires too much code, because lots of files include FrameMetrics.h, they all need to forward declare their used things.

I think your second option is a nice one, I will move the enum to FrameMetrics.h

Comment 20

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

Review commit: https://reviewboard.mozilla.org/r/221658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221658/

Comment 21

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

Review commit: https://reviewboard.mozilla.org/r/221678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221678/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8952452 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/221678/#review228082

Almost looks good to me. However, there are still some minor issues and looks like that you're new mozilla hacker. So, I'd like to check new patch again before requesting additional review to APZ peer.

::: dom/events/EventStateManager.cpp:2601
(Diff revision 1)
> -    if (checkIfScrollableY) {
> -      if (!CanVerticallyScrollFrameWithWheel(scrollFrame)) {
> +      return frameToScroll;
> +    }
> +
> +    // If the frame disregards the direction the user is trying to scroll, then
> +    // it should just bubbles the scroll event up to its parental scroll frame
> +    switch (WheelHandlingUtils::GetDisregardedWheelScrollDirection(scrollFrame)) {

Well, unfortunately, longer than 80 characters. Although, it becomes ugly:

>     switch (WheelHandlingUtils::GetDisregardedWheelScrollDirection(
>                                   scrollFrame)) {

::: dom/events/WheelHandlingHelper.h:18
(Diff revision 1)
>  #include "nsIFrame.h"
>  #include "nsPoint.h"
>  
>  class nsIScrollableFrame;
>  class nsITimer;
> +using mozilla::layers::DisregardedWheelScrollDirection;

I don't think that it should be in "layers" namespace because it's shared by dom/events.

So, please move the declaration to under "mozilla".

Then, this "using" must be unnecessary.

::: gfx/layers/FrameMetrics.h:28
(Diff revision 1)
>  namespace mozilla {
>  namespace layers {

So, please move the declaration between these lines.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:110
(Diff revision 1)
> -typedef mozilla::layers::AllowedTouchBehavior AllowedTouchBehavior;
>  typedef GeckoContentController::APZStateChange APZStateChange;
>  typedef GeckoContentController::TapType TapType;
>  typedef mozilla::gfx::Point Point;
>  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
> -using mozilla::gfx::PointTyped;
> +using mozilla::layers::DisregardedWheelScrollDirection;

I don't understand why this using statement is necessary here. Although it should be declared in mozilla namespace, but anyway, here is in mozilla::layers.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1972
(Diff revision 1)
> +  if (mX.CanScroll(aDelta.x)
> +      && DisregardedWheelScrollDirection::eHorizontal != disregardedDirection) {

nit: && operator should be in the last of the original line.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1976
(Diff revision 1)
> -  if (mY.CanScroll(aDelta.y) && mScrollMetadata.AllowVerticalScrollWithWheel()) {
> +  if (mY.CanScroll(aDelta.y)
> +      && DisregardedWheelScrollDirection::eVertical != disregardedDirection) {

ditto.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2723
(Diff revision 1)
> -    ParentLayerPoint adjustedDisplacement;
> -    bool forceVerticalOverscroll =
> +    bool forcesDisregardedDirectionOverscroll
> +      = ScrollSource::Wheel == aOverscrollHandoffState.mScrollSource;

nit: = should be in the last of the original line.

::: gfx/layers/ipc/LayersMessageUtils.h:275
(Diff revision 1)
> +// aParam.mDisregardedDirection is an enumeration value, it should be ensured to
> +// be of a fixed size, so convert it to SizeFixedDisregardedDirection.
> +using SizeFixedDisregardedDirection = uint8_t;

Please declare this before the declaration of DeisrgardedWheelScrollDirection like this:
https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/widget/EventForwards.h#87-88

Otherwise, only the size of DesiregardedWheelScrollDirection may be changed by other developers.

::: layout/base/nsLayoutUtils.cpp:9378
(Diff revision 1)
> -        EventStateManager::CanVerticallyScrollFrameWithWheel(aScrollFrame->GetParent()))
> -    {
> +      // @see mozilla::layers::DisregardedWheelScrollDirection to understand
> +      // what a disregarded scroll direction means

I don't think that this is necessary comment since our source code viewer is easy to find the declaration. (searchfox.org and dxr.mozilla.org)

Comment 23

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8952452 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/221678/#review228144

Got them fixed. A new patch will be uploaded soon

Comment 24

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

Review commit: https://reviewboard.mozilla.org/r/222232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222232/

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8952974 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/222232/#review228180

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:110
(Diff revision 1)
> -typedef mozilla::layers::AllowedTouchBehavior AllowedTouchBehavior;
>  typedef GeckoContentController::APZStateChange APZStateChange;
>  typedef GeckoContentController::TapType TapType;
>  typedef mozilla::gfx::Point Point;
>  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
> -using mozilla::gfx::PointTyped;
> +using mozilla::DisregardedWheelScrollDirection;

Why do you need this? Here is in mozilla::layers.

::: gfx/layers/apz/src/GenericScrollAnimation.h:25
(Diff revision 1)
>  
> +// If a direction is forced to overscroll, it means it's axis in that direction
> +// is locked, and scroll in that direction is treated as overscroll of an equal
> +// amount, which, for example, may then bubble up a scroll action to its parent,
> +// or may behave as whatever an overscroll occurence requires to behave
> +using DirectionForcedToOverscroll = mozilla::DisregardedWheelScrollDirection;

Hmm, this is hacky and must break the type-safe produced by enum class. But it's okay, perhaps.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8952974 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

Kartikaya Gupta:

Hi, could you review the APZ part? I don't have enough knowledge to review it.

Comment 27

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #25)
> Comment on attachment 8952974 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> https://reviewboard.mozilla.org/r/222232/#review228180
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:110
> (Diff revision 1)
> > -typedef mozilla::layers::AllowedTouchBehavior AllowedTouchBehavior;
> >  typedef GeckoContentController::APZStateChange APZStateChange;
> >  typedef GeckoContentController::TapType TapType;
> >  typedef mozilla::gfx::Point Point;
> >  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
> > -using mozilla::gfx::PointTyped;
> > +using mozilla::DisregardedWheelScrollDirection;
> 
> Why do you need this? Here is in mozilla::layers.
> 

It's likely I will still have something to write in this file later on for other bugs, because I would like to treat scroll handling for some time. I'll delete this using in the next patch

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8952974 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/222232/#review228206

This looks mostly fine, but I'd like to see the updated patch with the below comments addressed. Removing the enums in particular will affect a number of things (includes, comments, etc.) so I'd like to review the updated version.

::: gfx/layers/FrameMetrics.h:38
(Diff revision 1)
> + * text control frame should disregard wheel scroll in blcok-flow direction.
> + * We also provide an alias in a fixed size, because it is also used in IPC
> + * data transportation.
> + */
> +using SizeFixedDisregardedDirection = uint8_t;
> +enum class DisregardedWheelScrollDirection : SizeFixedDisregardedDirection

Instead of adding a new enum for this I'd prefer to use the existing ScrollDirection enum [1], and wrap it in a Maybe<> to get the eNone case. We do this in other places and it's safe for use in the IPC channels.

[1] https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/gfx/layers/LayersTypes.h#348

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
(Diff revision 1)
>  #endif
>  
>  namespace mozilla {
>  namespace layers {
>  
> -typedef mozilla::layers::AllowedTouchBehavior AllowedTouchBehavior;

Please put this line back. In general please don't make changes that are unrelated to the actual fix you are doing.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:110
(Diff revision 1)
> -typedef mozilla::layers::AllowedTouchBehavior AllowedTouchBehavior;
>  typedef GeckoContentController::APZStateChange APZStateChange;
>  typedef GeckoContentController::TapType TapType;
>  typedef mozilla::gfx::Point Point;
>  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
> -using mozilla::gfx::PointTyped;
> +using mozilla::DisregardedWheelScrollDirection;

Here too please leave the original using statement

::: gfx/layers/apz/src/GenericScrollAnimation.h:25
(Diff revision 1)
>  
> +// If a direction is forced to overscroll, it means it's axis in that direction
> +// is locked, and scroll in that direction is treated as overscroll of an equal
> +// amount, which, for example, may then bubble up a scroll action to its parent,
> +// or may behave as whatever an overscroll occurence requires to behave
> +using DirectionForcedToOverscroll = mozilla::DisregardedWheelScrollDirection;

Drop this as well, just use Maybe<ScrollDirection>

Comment 29

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8952974 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/222232/#review228206

> Instead of adding a new enum for this I'd prefer to use the existing ScrollDirection enum [1], and wrap it in a Maybe<> to get the eNone case. We do this in other places and it's safe for use in the IPC channels.
> 
> [1] https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/gfx/layers/LayersTypes.h#348

All new introduced types, DisregardedWheelScrollDirection, SizeFixedDisregardedDirection and typedefed DirectionForcedToOverscroll have all been refactored into Maybe<ScrollDirection>.

> Please put this line back. In general please don't make changes that are unrelated to the actual fix you are doing.

Recovered.

Comment 30

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

As for now, the scrollable direction with a mouse wheel for a single-line text
control is hard-coded; that is, only horizontal wheel scrolls are able to take
effect while vertical ones aren't. However, this isn't the desired case for
vertical writing mode, where the opposite case definitely suits better.

This commit refines the hard-coded scrollable direction for a single-line text
control to be writing-mode-adaptive.

Review commit: https://reviewboard.mozilla.org/r/222444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222444/

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/222444/#review228370

Much better, thanks! I have some minor comments below but the patch is functionally fine so giving r+.

::: gfx/layers/FrameMetrics.h:955
(Diff revision 1)
> +  // code which defines mDisregardedDirection.
> +  Maybe<ScrollDirection> GetDisregardedDirection() const {
> +    return mDisregardedDirection;
> +  }
> +  void
> +  SetDisregardedDirection(Maybe<ScrollDirection> aValue) {

Change the parameter here to "const Maybe<ScrollDirection>& aValue"

::: gfx/layers/FrameMetrics.h:1016
(Diff revision 1)
>  
> +  // The disregarded direction means the direction which is disregarded anyway,
> +  // even if the scroll frame overflows in that direction and the direction is
> +  // specified as scrollable. This could happen in some scenarios, for instance,
> +  // a single-line text control frame should disregard wheel scroll in
> +  // its blcok-flow direction even if it overflows in that direction.

typo: blcok -> block

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2724
(Diff revision 1)
> -    ParentLayerPoint adjustedDisplacement;
> -    bool forceVerticalOverscroll =
> -      (aOverscrollHandoffState.mScrollSource == ScrollSource::Wheel &&
> -       !mScrollMetadata.AllowVerticalScrollWithWheel());
> -    bool yChanged = mY.AdjustDisplacement(displacement.y, adjustedDisplacement.y, overscroll.y,
> -                                          forceVerticalOverscroll);
> -    bool xChanged = mX.AdjustDisplacement(displacement.x, adjustedDisplacement.x, overscroll.x);
> +    bool forcesDisregardedDirectionOverscroll =
> +      ScrollSource::Wheel == aOverscrollHandoffState.mScrollSource;
> +    bool forcesVerticalOverscroll = false;
> +    bool forcesHorizontalOverscroll = false;
> +    if (forcesDisregardedDirectionOverscroll) {
> +      Maybe<ScrollDirection> disregardedDirection =
> +        mScrollMetadata.GetDisregardedDirection();
> +      if (disregardedDirection) {
> +        switch (disregardedDirection.ref()) {
> +          case ScrollDirection::eVertical:
> +            forcesVerticalOverscroll = true;
> +            break;
> +          case ScrollDirection::eHorizontal:
> +            forcesHorizontalOverscroll = true;
> +            break;
> +        }
> +      }
> +    }

This block would be easier to read if you simplified it to this:

bool forcesVerticalOverscroll =
  ScrollSource::Wheel == aOverscrollHandoffState.mScrollSource &&
  mScrollMetadata.GetDisregardedDirection() == Some(ScrollDirection::eVertical);
bool forcesHorizontalOverscroll =
  ScrollSource::Wheel == aOverscrollHandoffState.mScrollSource &&
  mScrollMetadata.GetDisregardedDirection() == Some(ScrollDirection::eHorizontal);

While it does have slightly redundant clauses the compiler will probably optimize those out, and even if it doesn't, this isn't particularly performance sensitive code so I think the improved readability would be more useful to have.

::: gfx/layers/apz/src/Axis.h:87
(Diff revision 1)
>     * to account for overscroll (which might decrease the displacement; this is
>     * to prevent the viewport from overscrolling the page rect), and axis locking
>     * (which might prevent any displacement from happening). If overscroll
>     * ocurred, its amount is written to |aOverscrollAmountOut|.
> -   * The |aDisplacementOut| parameter is set to the adjusted
> -   * displacement, and the function returns true iff internal overscroll amounts
> +   * The |aDisplacementOut| parameter is set to the adjusted displacement, and
> +   * the function returns true if internal overscroll amounts were changed.

the "iff" was not a typo, it stands for "if and only if". Please restore it (or replace it with "if and only if").

::: gfx/layers/apz/src/GenericScrollAnimation.cpp:24
(Diff revision 1)
>  GenericScrollAnimation::GenericScrollAnimation(AsyncPanZoomController& aApzc,
>                                                 const nsPoint& aInitialPosition,
>                                                 const ScrollAnimationBezierPhysicsSettings& aSettings)
>    : mApzc(aApzc)
>    , mFinalDestination(aInitialPosition)
> -  , mForceVerticalOverscroll(false)
> +  , mDirectionForcedToOverscroll(Nothing())

Maybe<> is initialized to Nothing() by default so technically you don't need this initializer.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

Hmm, don't you update old commit with |hg histedit|? If you do it, r+ will be carried over automatically.

Comment 33

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #32)
> Comment on attachment 8953170 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> Hmm, don't you update old commit with |hg histedit|? If you do it, r+ will
> be carried over automatically.

Thanks. I didn't know of that. I used to use strip --keep and then redo a commit

Comment 34

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222444/diff/1-2/

Comment 35

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #32)
> Comment on attachment 8953170 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> Hmm, don't you update old commit with |hg histedit|? If you do it, r+ will
> be carried over automatically.

It still changes r+ to r?, but now the review is much clearer and only shows the diff

Comment 36

[張俊芝(ZHANG Junzhi)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #32)
> Comment on attachment 8953170 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> Hmm, don't you update old commit with |hg histedit|? If you do it, r+ will
> be carried over automatically.

Please do me a favour to trigger a try build after you give it a r+. Thanks.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

https://reviewboard.mozilla.org/r/222444/#review228630

::: gfx/layers/FrameMetrics.h:954
(Diff revisions 1 - 2)
>    // code which defines mDisregardedDirection.
>    Maybe<ScrollDirection> GetDisregardedDirection() const {
>      return mDisregardedDirection;
>    }
>    void
> -  SetDisregardedDirection(Maybe<ScrollDirection> aValue) {
> +  SetDisregardedDirection(const Maybe<ScrollDirection> aValue) {

You missed the "&" - it's supposed to be a const-ref parameter, so the compiler doesn't make a copy of it needlessly. See SetOverscrollBehaviour and other functions in this file, for example.

Comment 38

[張俊芝(ZHANG Junzhi)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> Comment on attachment 8953170 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> https://reviewboard.mozilla.org/r/222444/#review228630
> 
> ::: gfx/layers/FrameMetrics.h:954
> (Diff revisions 1 - 2)
> >    // code which defines mDisregardedDirection.
> >    Maybe<ScrollDirection> GetDisregardedDirection() const {
> >      return mDisregardedDirection;
> >    }
> >    void
> > -  SetDisregardedDirection(Maybe<ScrollDirection> aValue) {
> > +  SetDisregardedDirection(const Maybe<ScrollDirection> aValue) {
> 
> You missed the "&" - it's supposed to be a const-ref parameter, so the
> compiler doesn't make a copy of it needlessly. See SetOverscrollBehaviour
> and other functions in this file, for example.

Thank you for your careful review.

Comment 39

[張俊芝(ZHANG Junzhi)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> Comment on attachment 8953170 [details]
> Bug 1438794 - Makes single-line text controls in vertical-writing mode
> vertically scrollable if they overflow vertically; and makes them
> horizontally unscrollable no matter whether they overflow horizontally.
> 
> https://reviewboard.mozilla.org/r/222444/#review228630
> 
> ::: gfx/layers/FrameMetrics.h:954
> (Diff revisions 1 - 2)
> >    // code which defines mDisregardedDirection.
> >    Maybe<ScrollDirection> GetDisregardedDirection() const {
> >      return mDisregardedDirection;
> >    }
> >    void
> > -  SetDisregardedDirection(Maybe<ScrollDirection> aValue) {
> > +  SetDisregardedDirection(const Maybe<ScrollDirection> aValue) {
> 
> You missed the "&" - it's supposed to be a const-ref parameter, so the
> compiler doesn't make a copy of it needlessly. See SetOverscrollBehaviour
> and other functions in this file, for example.

I will dig deeper into Maybe

Comment 40

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222444/diff/2-3/

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Perfect, thanks!

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8953170 [details]
Bug 1438794 - Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally.

All green on tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=dec59e99dc013f85c764910620435300665e5cb6

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/789c6aac53c6
Makes single-line text controls in vertical-writing mode vertically scrollable if they overflow vertically; and makes them horizontally unscrollable no matter whether they overflow horizontally. r=kats

Comment 44

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/789c6aac53c6