Closed Bug 1161389 Opened 9 years ago Closed 9 years ago

Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(1 file, 3 obsolete files)

Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret. (v4)
4.07 KB, patch
roc
: review+
Details | Diff | Splinter Review 
Originally, this was intended to be done in bug 1110039 Part 6.

Quote roc suggestion from bug 1110039 comment 40:
"I think we could fix this by modifying nsCanvasFrame instead, to walk the frame children of the custom content container in nsCanvasFrame and call BuildDisplayList on them directly, skipping the caret elements when !IsBuildingCaret."

However, I need more investigation regarding the location of the custom content container frame in the frame tree.
Blocks: 1161392
Move the r- patch from bug 1110039 Part 6 for a record.
In bug 1110039, we added the AccessibleCaret element as an anonymous content to nsCanvasFrame's mCustomContentContainer. We need to walk all the children of the custom content container to find AcceesibleCaret frame in order to skip it when nsDisplayListBuilder::IsBuildingCaret() is true. To do that, we need to locate the custom content container frame in frame tree.

However, I found the location of custom content container frame in frame tree is not what I expected.

Reproduce steps:
1. Enable "layout.accessiblecaret.enabled" (For inserting anonymous contents to the custom content container)
2. firefox -layoutdebug about:blank
3. Dump the frame tree.

Actual results:
The custom content container frame is a child of nsBlockFrame(html), which is a sibling to the frames of web contents.

Expected results:
The custom content container frame is a child of nsCanvasFrame. I got this results by removing HideCustomContentContainer() in [1] added by bug 1116714.

See full frame tree dump for the actual and expected results here. https://pastebin.mozilla.org/8832445

Mats, it seems that HideCustomContentContainer() will prevent the custom content container frame from being generated when the frame tree is constructed [2]. Once the container has some children, it gets reframed like regular web content. Do you any ideas to make the expected results without removing HideCustomContentContainer() in [1]? I feel it's a bug to find custom content container frame in nsBlockFrame(html)'s children.

[1] https://hg.mozilla.org/mozilla-central/file/102d0e9aa9e1/layout/generic/nsCanvasFrame.cpp#l165
[2] https://hg.mozilla.org/mozilla-central/file/102d0e9aa9e1/layout/base/nsCSSFrameConstructor.cpp#l5519
Flags: needinfo?(mats)
This patch works only if the custom content container frame is a child of
nsCanvasFrame.
Attachment #8601278 - Attachment is obsolete: true
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> Reproduce steps:

I can't reproduce with these steps.  I guess it's because bug 1110039
hasn't landed yet.  Can you provide a rollup patch for those patches
against trunk?

> Mats, it seems that HideCustomContentContainer() will prevent the custom
> content container frame from being generated when the frame tree is
> constructed [2].

Yes, that's intentional.  We don't want to have extra frames when they
are not needed, for performance and debugging reasons.

> I feel it's a bug to find
> custom content container frame in nsBlockFrame(html)'s children.

Are you sure you're using position:fixed ?
Flags: needinfo?(mats)
Hi Mats,

Bug 1110039 was landed, and AccessibleCaret is available on m-c. Would you try the steps again in comment 2? After flipping "layout.accessiblecaret.enabled" to true in step 1, You'll need to restart browser or reload the current page before dumping the frame tree.

> Are you sure you're using position:fixed ?
Yes. The custom content container is position:fixed [1].

One quick hack I could think of is to skip HideCustomContentContainer() in [2] if AccessibleCaret pref is enabled since we know that the AccessibleCaret elements will be inserted into that container anyway. 

[1] http://hg.mozilla.org/mozilla-central/file/bedce1b405a3/layout/style/ua.css#l524
[2] http://hg.mozilla.org/mozilla-central/file/bedce1b405a3/layout/generic/nsCanvasFrame.cpp#l164
Flags: needinfo?(mats)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) (PTO, back on 5/26) from comment #2)
> However, I found the location of custom content container frame in frame
> tree is not what I expected.
> 
> Reproduce steps:
> 1. Enable "layout.accessiblecaret.enabled" (For inserting anonymous contents
> to the custom content container)
> 2. firefox -layoutdebug about:blank
> 3. Dump the frame tree.
> 
> Actual results:
> The custom content container frame is a child of nsBlockFrame(html), which
> is a sibling to the frames of web contents.

That's correct. Actually there's two frames for the custom container here: a placeholder, which is a child of nsBlockFrame(html), and the fixed-pos frame which is a child of the viewport. Any children of the custom container will be inserted under the fixed-pos frame.
Flags: needinfo?(mats)
Comment on attachment 8607408 [details] [diff] [review]
Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret. (v3)

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

::: layout/generic/nsCanvasFrame.cpp
@@ +164,5 @@
>  
>    // Only create a frame for mCustomContentContainer if it has some children.
> +  // When AccessibleCaret is enabled, it will insert children into
> +  // mCustomContentContainer. No need to hide the container.
> +  if (len == 0 && !PresShell::AccessibleCaretEnabled()) {

I don't understand why this change is needed.

@@ +520,5 @@
> +        // Do nothing. Skip TouchCaret/SelectionCarets frame.
> +        continue;
> +      }
> +
> +      if (kid->GetContent() == mCustomContentContainer) {

Given the custom content container placeholder frame is a grandchild of the nsCanvasFrame, I don't see how this works.

This is a bit harder than I thought.

I think this will work:
-- In nsDisplayListBuilder::MarkFramesForDisplayList, if aDirtyFrame is the viewport (easy check: just check if it has a null parent) and !IsBuildingCaret(), then check each element of aFrames to see if it's the frame for a caret element and skip it if so. That should ensure those elements aren't painted, since when we reach the placeholder, we won't paint anything if MarkOutOfFlowFrameForDisplay wasn't already called on its out-of-flow.
Attachment #8607408 - Flags: review?(roc) → review+
Comment on attachment 8607408 [details] [diff] [review]
Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret. (v3)

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

oops sorry
Attachment #8607408 - Flags: review+ → review-
Comment on attachment 8607408 [details] [diff] [review]
Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret. (v3)

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

::: layout/generic/nsCanvasFrame.cpp
@@ +164,5 @@
>  
>    // Only create a frame for mCustomContentContainer if it has some children.
> +  // When AccessibleCaret is enabled, it will insert children into
> +  // mCustomContentContainer. No need to hide the container.
> +  if (len == 0 && !PresShell::AccessibleCaretEnabled()) {

This is to make the custom content container frame being a child of nsCanvasFrame like touch/selection-carets frames, which should be a wrong assumption base on your comment 7.
Implement the solution given in comment 8. Since a accessible-caret frame is a
grandchild of the viewport frame, I check only !IsBuildingCaret() in
nsDisplayListBuilder::MarkFramesForDisplayList.
Attachment #8607408 - Attachment is obsolete: true
Attachment #8610401 - Flags: review?(roc)
Comment on attachment 8610401 [details] [diff] [review]
Skip AccessibleCaret frame if nsDisplayListBuilder doesn't build caret. (v4)

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

Ah, great stuff!
Attachment #8610401 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/ad884eef3481
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: