1358102 - Certain CSS transitions are failing since v48(?)

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[chuck]

Attachment: FF-images-for-bug-report.pdf

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Visit
http://www.arrowtheme.com/drthemes/?theme=illusion-drupal&frame=http://arrowtheme.com/illusiondemo/

Attempt to have tertiary menu items appear when hovering over secondary items with submenu items.


Actual results:

Tertiary flyouts do not appear properly (1 or 2 pixels wide) in versions > 47x


Expected results:

Tertiary submenu should appear normally when hovering over secondary menu item which has associated submenu.

Comment 1

[Alice0775 White]

Attachment: semi-reduced html

Comment 2

[Alice0775 White]

At least, there are two regression window.

#1 Regression window(1st menu and 2nd sub-menu would not open):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22360424ed1532c73c93aab85bfd9173899956f8&tochange=8084077bd124bc25f521fd31fe51a9d1c508c68a

Regressed by: ac7d0d74ced6	Thinker K.F. Li — Bug 1208673 - Do HitTest with skipping non-leaf preserve-3d transform items. r=roc

#2 Regression window (partial fix: 1st menu open, but 2nd sub-menu would not):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6c49bb716593014c07d3736ea2124f74bfe3bb06&tochange=691f2e687e46d1b166b88b0a3f9dbd480bff9afa

Regressed by: f950b7a04741	Thinker K.F. Li — Bug 1226904 - Fix boundary checking for leaves collecting. r=roc

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

It seems both regression window are from Thinker.

Thinker, could you have a look at this regression?

Comment 4

[Thinker Li [:sinker]]

Get it! I would take time on this in this week.

Comment 5

[Thinker Li [:sinker]]

The submenu are clipped by the visible region of the parent for some reason.  I will look into it deeper.

Comment 6

[Thinker Li [:sinker]]

NOTE:
One of problems is the display items of sub-menus are not built for their visual overflow is out of the dirty rect.
The dirty rect is bound by the menu item of the main menu, for it is with float property.
The parent of the frame of the main menu item calls |nsDisplayListBuilder::MarkOutOfFlowFrameForDisplay|
against all children, including sub-menus, in |mFloats| setting a |OutOfFlowData| with |mDirtyRect| included.
The |mDirtyRect| would be used for building display list as the dirt rect.
The |NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO| flag bit can be used to fixed the problem.

The other problem is that even display items are created, the the content of items are not shown immediately.
It is another problem I will look into in following days.

Comment 7

[Thinker Li [:sinker]]

NOTE:
For some reason, the frames of first level sub_menu do not |UpdateOverflow()|
when the frames of second level sub_menu are transformed.
I attached the process with gdb to force them |UpdateOverflow()|, then second level sub_menus
are shown correctly.

So, the question is how to make |UpdateOverflow()| being called correctly.

Comment 8

[Thinker Li [:sinker]]

Attachment: fix-change-checking.diff

The frames, except establisers, participating preserve-3D rendering context was
skipped when propagate change of overflow along the frame tree to the root.
Leaves of 3D rendering context are also skipped to incorrect values returned by
|GetOverflowAreasRelativeToSelf()| and incorrect overflow computed by
|nsIFrame::ComputePreserve3DChildrenOverflow()|.

Comment 9

[Thinker Li [:sinker]]

Chuck, would you try the patch?

Comment 10

[Thinker Li [:sinker]]

Alice, could you confirm if the patch fix the problem?

Comment 11

[Alice0775 White]

I built 32bit inbound from 4d3fe42a592d + attachment 8878647 [details] [diff] [review] in locally windows10 64bit.

Menu(1st and 2nd) seems to be expand correctly.
However, no menu labels are rendered --- this is regressed by Bug 1361970 :(

Comment 12

[Thinker Li [:sinker]]

Attachment: OverflowChangedTracker does not skip leaf frames of a 3D rendering context

This patch make OverflowChangedTracker to stop skipping leaf frame of 3D rendering context.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8878745 [details] [diff] [review]
OverflowChangedTracker does not skip leaf frames of a 3D rendering context

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

::: layout/generic/nsFrame.cpp
@@ +9077,5 @@
> +    bool combines3DTransformWithAncestors =
> +      Combines3DTransformWithAncestors(disp);
> +    if (MOZ_UNLIKELY(combines3DTransformWithAncestors &&
> +                     !Extend3DContext(disp, effectSet))) {
> +      // Preserve-3d leaf frame!!

Intermediate preserve-3d frames can have non-empty pre-transform overflow areas too, since they can have children that don't participate in preserve-3d.

Do we need to handle those here too?

Comment 14

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Intermediate preserve-3d frames can have non-empty pre-transform overflow
> areas too, since they can have children that don't participate in
> preserve-3d.
> 
> Do we need to handle those here too?

If a descendant of an intermediate frame does not participate in preserve-3d,
there must has a leaf frame in-between itself and the intermediate frame.
So, it can not happen that a frame is a direct child of an intermediate frame
and not participate preserve-3d.  It at least should be a leaf frame.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Are you sure about that?

nsIFrame::Combines3DTransformWithAncestors returns false if the parent has preserve-3d (preserve-3d root or intermediate frame), but isn't transformed itself.

Those frames should contribute pre-transform overflow area to their parent, which is an intermediate (or the root) preserve-3d frame.

Comment 16

[Thinker Li [:sinker]]

Err! I didn't notice that.   |IsPreserve3DLeaf()| seems need to be changed by this confusing definition.
I think I need to take time to check existing code calling Combines3DTransformWithAncestors(), to see
if there is any of them depending on wrong assumption.

Comment 17

[Thinker Li [:sinker]]

Attachment: study-comments.diff

This patch comprises comments and opinions on Combines3DTransformWithAncestors().
I checked all functions calling to Combines3DTranformWithAncestors() to see if they are reasonable or what can be changed.

Comment 18

[Thinker Li [:sinker]]

Attachment: change-meaning-combines.diff

I try to change Combines3DTransformWithAncestors().  It has pass all reftests. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ea0f631497cbe7df4e445ab7ae782a62ae3e15

I am studying what the side-effects are.

Comment 19

[Astley Chen (inactive)]

Thinker,it seems you are working on it.
Feel free to let know if you are not gonna work on this bug.

Comment 20

[Mike Taylor [:miketaylr]]

What's the status of this bug Thinker?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Still reproduces on trunk.

Comment 22

[Bogdan Maris, Desktop QA]

Still reproducible in 63.0b8 and Latest Nightly 64.0b1.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix in 63. We could still take a patch for 65 and potentially for 64.

Comment 24

[Hani Yacoub, Desktop QA]

Reproducible on Windows 7 x32 using this testcase also "https://bug1358102.bmoattachments.org/attachment.cgi?id=8867531" on Firefox Nightly 68.0a1, Firefox 67.0b17 and Firefox 66.0.4

Comment 25

[Gregory Pappas [:gregp]]

Fixed somewhere in this range.