Open Bug 1601013 Opened 5 years ago Updated 2 years ago

Recursive backplate functions for visibility, locating text frames should be consolidated for performance

Categories

(Core :: Layout, enhancement, P4)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

People

(Reporter: morgan, Unassigned)

References

Details

After 1594099, we'll have four sets of recursive/tree-recursive functions in nsBlockFrame.cpp; these are called from the same line iterator and arguably touch the same frames. We should consolidate them to save compute time.

The functions I'm referring to are:

  • GetFrameTextArea
  • GetLineTextArea
  • FrameHasVisibleInlineContent
  • LineHasVisibleInlineContent

The (get)Line* versions can probably be squashed together, and same with (get)Frame*.

There's some larger-scope reworking to be done here in consolidating due to the fact that the visibility functions return bools that control the outer line iteration, and the textArea functions return nsRects that are unioned in the iteration. Squashing these functions likely requires reworking the outer line iteration, too, not just the functions themselves.

Priority: -- → P4

I think there may be some correctness issues that can be addressed by this combination, too.

For example, consider this testcase:

data:text/html,<body style="background-image:linear-gradient(blue,blue)"><span><span>Visible</span><span style="visibility:hidden">invisible<span></span>

In current Nightly (and I think in Nightly with bug 1594099's patch), we'll backplate the trailing "invisible" text, and that's probably incorrect. (We'll do it even after bug 1594099's patch because the GetFrameTextArea API in that patch (called on the outermost span here) will traverse both of its children and union their text area, even though one of the text nodes has visibility:hidden.

Really, GetFrameTextArea should probably look something like

  if (aFrame->IsTextFrame()) {
    if (aFrame->StyleVisibility()->IsVisible()) { // <--- This line is new
      textArea = aFrame->GetVisualOverflowRect();
    }
  } else if ([...etc...]

But it probably makes sense to make this correctness fix as part of this merging, which is why I'm leaving it as note here rather than a review nit on bug 1594099.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.