Closed Bug 1005937 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(1 file, 4 obsolete files)

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/
+ Changes elements of touch-action property
Attachment #8417454 - Flags: review?(mak77)
Attachment #8417454 - Flags: review?(gavin.sharp)
Attachment #8417454 - Flags: review?(dbaron)
Attachment #8417454 - Flags: review?(bugs)
Attachment #8417454 - Flags: review?(bugmail.mozilla)
Attachment #8417454 - Flags: feedback?(romaxa)
Attachment #8417454 - Flags: feedback?(nicklebedev37)
Attachment #8417454 - Flags: review?(mbrubeck)
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;
}
Attachment #8417454 - Flags: feedback?(nicklebedev37) → feedback-
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 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.
Attachment #8417454 - Flags: review?(mak77)
Attachment #8417454 - Flags: review?(gavin.sharp)
Attachment #8417454 - Flags: review?(dbaron)
Attachment #8417454 - Flags: review?(bugs)
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.
Attachment #8417454 - Flags: review?(mbrubeck)
Attachment #8417454 - Flags: review?(dbaron)
Attachment #8417454 - Flags: review?(bugmail.mozilla)
Status: UNCONFIRMED → ASSIGNED
Component: General → Widget
Ever confirmed: true
Product: Firefox → Core
Assignee: nobody → alessarik
Blocks: 795567
+ Changes according comments
Attachment #8417454 - Attachment is obsolete: true
Attachment #8417454 - Flags: review?(dbaron)
Attachment #8417454 - Flags: feedback?(romaxa)
Attachment #8418051 - Flags: review?(dbaron)
Attachment #8418051 - Flags: feedback?(oleg.romashin)
Attachment #8418051 - Flags: feedback?(nicklebedev37)
(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 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);
> }
>
Attachment #8418051 - Flags: feedback?(oleg.romashin) → feedback+
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
Attachment #8418051 - Flags: review?(dbaron) → review+
(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.
+ Changes according with comments
Attachment #8418051 - Attachment is obsolete: true
Attachment #8418051 - Flags: feedback?(nicklebedev37)
Attachment #8418690 - Flags: review?(dbaron)
Attachment #8418690 - Flags: feedback?(nicklebedev37)
(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.
(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 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-.
Attachment #8418690 - Flags: review?(dbaron) → review-
+ 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).
Attachment #8418690 - Attachment is obsolete: true
Attachment #8418690 - Flags: feedback?(nicklebedev37)
Attachment #8422221 - Flags: review?(dbaron)
Attachment #8422221 - Flags: feedback?(nicklebedev37)
Comment on attachment 8422221 [details] [diff] [review]
change_touch_action_behavior_ver4.diff

># HG changeset patch
># Parent 3758a1906d7e7b96b516e75750ed8dcafa006737
># User Maksim Lebedev <alessarik@gmail.com>
>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
Attachment #8422221 - Flags: review?(dbaron) → review+
+ Changes according with comments
Attachment #8422221 - Attachment is obsolete: true
Attachment #8422221 - Flags: feedback?(nicklebedev37)
Attachment #8428556 - Flags: feedback?(nicklebedev37)
Attachment #8428556 - Flags: feedback?(nicklebedev37) → feedback+
Looks like issues on TRY server are not related with my patch.
If nobody have objections, I put "checkin" keyword to this bug.
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9adf12b93052
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.