1005937 - Touch-action property should apply to all elements except non-replaced inline elements

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Maksim Lebedev]

According specification:
The touch-action CSS property
Applies to: all elements except: non-replaced inline elements, table rows, row groups, table columns, and column groups

Test page: http://jsfiddle.net/S9C5Y/3/embedded/result/

Comment 1

[Maksim Lebedev]

Attachment: change_touch_action_behavior_ver1.diff

+ Changes elements of touch-action property

Comment 2

[Nick Lebedev [:nl]]

Comment on attachment 8417454 [details] [diff] [review]
change_touch_action_behavior_ver1.diff

Review of attachment 8417454 [details] [diff] [review]:
-----------------------------------------------------------------

Do the provided changes cover all cases (span, strong, i, em, tr, td... elements)?

I thought that a condition would look like smth following:
bool isReplacedInlineElement = (aFrame->IsFrameOfType(nsIFrame::eLineParticipant) || aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)) &&
                               (aFrame->IsFrameOfType(nsIFrame::eReplacedContainsBlock) || aFrame->IsFrameOfType(nsIFrame::eReplaced));
bool isTableElement = aFrame->IsFrameOfType(nsIFrame::eTablePart);

if (isReplacedInlineElement || isTableElement ) {
     return NS_STYLE_TOUCH_ACTION_AUTO;
}

Comment 3

[Nick Lebedev [:nl]]

Sorry, made an error in condition,
was:
bool isReplacedInlineElement = (aFrame->IsFrameOfType(nsIFrame::eLineParticipant) || aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)) &&
                               (aFrame->IsFrameOfType(nsIFrame::eReplacedContainsBlock) || aFrame->IsFrameOfType(nsIFrame::eReplaced));

need:
bool isNonReplacedInlineElement = (aFrame->IsFrameOfType(nsIFrame::eLineParticipant) || aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)) && !aFrame->IsFrameOfType(nsIFrame::eReplacedContainsBlock) && !aFrame->IsFrameOfType(nsIFrame::eReplaced);

Comment 4

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

Comment on attachment 8417454 [details] [diff] [review]
change_touch_action_behavior_ver1.diff

Review of attachment 8417454 [details] [diff] [review]:
-----------------------------------------------------------------

You really don't need that many reviewers for this change.

Comment 5

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

Comment on attachment 8417454 [details] [diff] [review]
change_touch_action_behavior_ver1.diff

Review of attachment 8417454 [details] [diff] [review]:
-----------------------------------------------------------------

Actually looking at the change I don't really know what all the different frame types are and how to isolate the elements you're looking for. Moving review back to dbaron.

Comment 6

[Maksim Lebedev]

Attachment: change_touch_action_behavior_ver2.diff

+ Changes according comments

Comment 7

[Maksim Lebedev]

(In reply to Nick Lebedev [:nl] from comment #3)
> need:
> bool isNonReplacedInlineElement =
> (aFrame->IsFrameOfType(nsIFrame::eLineParticipant) ||
> aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)) &&
> !aFrame->IsFrameOfType(nsIFrame::eReplacedContainsBlock) &&
> !aFrame->IsFrameOfType(nsIFrame::eReplaced);
I am confusing a bit with comments at
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.h#109
As I correct understand, if flag "eReplaced" will be removed from nsTextFrame,
in this case check "need:" will be incorrect.

Comment 8

[Oleg Romashin (MS)]

Comment on attachment 8418051 [details] [diff] [review]
change_touch_action_behavior_ver2.diff

>+  // non-replaced inline elements, table rows, row groups, table columns, and column groups
>+  bool isNonReplacedInlineElement = aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer)
>+                                    && aFrame->IsFrameOfType(nsIFrame::eLineParticipant);
                               nit:   ^ should go to the previous line.

>+  bool isTableElement = aFrame->IsFrameOfType(nsIFrame::eTablePart);
>+  if (isNonReplacedInlineElement || isTableElement) {
>     return NS_STYLE_TOUCH_ACTION_AUTO;
>   }
> 
>   return (aFrame->GetContent()->GetPrimaryFrame()->StyleDisplay()->mTouchAction);
> }
>

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8418051 [details] [diff] [review]
change_touch_action_behavior_ver2.diff

I'm reviewing this relative to the spec at
https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html#the-touch-action-css-property

eLineParticipant and eBidiInlineContainer differ in the following ways:
 * text frames are not eBidiInlineContainer (though they'll never have
   a value other than 'auto' anyway, since the property is not inherited
   and they can't be styled directly)
 * mathml container frames are not eBidiInlineContainer
 * br frames are not eBidiInlineContainer
 * floating ::first-letter frames are not eLineParticipant (but
   ::first-letter can't be styled with a value other than 'auto' anyway,
   since the property is not inherited and is not declared as
   CSS_PROPERTY_APPLIES_TO_FIRST_LETTER in nsCSSPropList.h)

They both include:
 * inline frames
 * ::first-line related frames
 * non-floating ::first-letter

Based on that, I think you want to drop the
"aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer) &&" since I think
both mathml frames and br frames should act as non-replaced inline
elements for this purpose.

(If you weren't to remove it, it should also match Mozilla style, which
puts && at end of line.)

>+  bool isTableElement = aFrame->IsFrameOfType(nsIFrame::eTablePart);

This also counts table cells (though, oddly, not table captions), which
you don't want to be counted.

Instead, put the result of aFrame->GetStyleDisplay() in a variable
"const nsStyleDisplay* disp = aFrame->GetStyleDisplay()" before this
check and check:
  bool isTableRowOrColumn = disp->IsInnerTableStyle() &&
                            disp->mDisplay != NS_STYLE_DISPLAY_TABLE_CELL &&
                            disp->mDisplay != NS_STYLE_DISPLAY_TABLE_CAPTION;

Use disp at the end of the function too.

You should also replace all occurrences of
aFrame->GetContent()->GetPrimaryFrame() in the function with just aFrame
(and just remove the one that is then a duplicate null check).  That
will just make things broken once we have features like fragment
styling.

Also, please wrap the comment at less than 80 characters.

r=dbaron with that

Comment 10

[Maksim Lebedev]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #9)
> >+  bool isTableElement = aFrame->IsFrameOfType(nsIFrame::eTablePart);
> This also counts table cells (though, oddly, not table captions), which
> you don't want to be counted.
Looks like nsTableCellFrame have nsBlockFrame with touch-action, which can have non-auto value.
At least test shows that this check is enough.

Comment 11

[Maksim Lebedev]

Attachment: change_touch_action_behavior_ver3.diff

+ Changes according with comments

Comment 12

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=a64658ee273d

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Maksim Lebedev from comment #10)
> Looks like nsTableCellFrame have nsBlockFrame with touch-action, which can
> have non-auto value.
> At least test shows that this check is enough.

I don't understand.  The block frame inside matches the ::-moz-table-cell pseudo-element, which doesn't explicitly inherit 'touch-action', and touch-action isn't an inherited property.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #9)
> You should also replace all occurrences of
> aFrame->GetContent()->GetPrimaryFrame() in the function with just aFrame
> (and just remove the one that is then a duplicate null check).  That
> will just make things broken once we have features like fragment
> styling.

You also missed the above comment.  Once you address it you will need to fix the table cell issue, but right now it's covering it up.

Also, it would have been nice to keep the isNonReplacedInlineElement and isTableElement variables, both as self-documenting code, and to keep the line less than 80 characters.  At a minimum, wrap the line.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8418690 [details] [diff] [review]
change_touch_action_behavior_ver3.diff

Normally you don't need to ask for review again once you have review+.  However, since you didn't address all the comments, this one gets review-.

Comment 16

[Maksim Lebedev]

Attachment: change_touch_action_behavior_ver4.diff

+ Changes according with comments

(In reply to David Baron [:dbaron] (Away/Busy May 10-22) (UTC+9) from comment #13)
> (In reply to Maksim Lebedev from comment #10)
> > Looks like nsTableCellFrame have nsBlockFrame with touch-action, which can
> > have non-auto value.
> > At least test shows that this check is enough. 
> I don't understand.  The block frame inside matches the ::-moz-table-cell
> pseudo-element, which doesn't explicitly inherit 'touch-action', and
> touch-action isn't an inherited property.
Inspite of non-inherited touchAction property we have ContentHelper, which goes from element to all his parents and read all they touchAction properties. And I mean when we find nsBlockFrame (with non-auto touchAction) we go to parent (for example nsTableCellFrame).

Comment 17

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=5881fbc5e6d1

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8422221 [details] [diff] [review]
change_touch_action_behavior_ver4.diff

># HG changeset patch
># Parent 3758a1906d7e7b96b516e75750ed8dcafa006737
># User Maksim Lebedev <[e-mail]>
>Change behavior of touch-action property. r=dbaron

Change the commit message to:

Bug 1005937 - Make the 'touch-action' CSS property apply to all elements except non-replaced inline elements and table rows, columns, and row/column-groups.

>+  const nsStyleDisplay* style = aFrame->StyleDisplay();

Call this variable disp instead of style.

>+  bool isTableElement = style->IsInnerTableStyle() &&
>+                          style->mDisplay != NS_STYLE_DISPLAY_TABLE_CELL &&
>+                          style->mDisplay != NS_STYLE_DISPLAY_TABLE_CAPTION;

two spaces less of indentation here, so all three occurrences of "disp" line up with each other.

r=dbaron with that

Comment 19

[Maksim Lebedev]

Attachment: change_touch_action_behavior_ver5.diff

+ Changes according with comments

Comment 20

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=e5369261ba61

Comment 21

[Maksim Lebedev]

https://tbpl.mozilla.org/?tree=Try&rev=2607a4773095

Comment 22

[Maksim Lebedev]

Looks like issues on TRY server are not related with my patch.
If nobody have objections, I put "checkin" keyword to this bug.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, it's ok.

You should probably do try pushes that trigger fewer builds, though, which uses fewer build resources and reduces the chance of hitting intermittent oranges.  You can also retrigger individual failed builds through TBPL (the blue + icon) rather than do an entirely new try push.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9adf12b93052

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9adf12b93052