1795529 - Clean up some Feature Callout position oversights

Status: RESOLVED WORKSFORME

(Firefox :: Messaging System, defect, P2)

Description

[Shane Hughes [:aminomancer]]

1. When Firefox View first loads, the feature callout doesn't fade in smoothly. It starts already rendered at the top for some reason, then abruptly moves on positioning.

2. When a message initially loads, it's possible for it to be positioned before the message's strings have been translated, because Fluent operations are asynchronous. When that happens, the callout's dimensions may not be correct at the time the callout is initially positioned. This can cause it to be positioned incorrectly, since some of the positioners use the callout's height/width to compute the final position.

   I think this might be the cause of some odd intermittent test failures, since I think Fluent strings can be cached at the document level. This can be fixed by forcing synchronous translation when we position the callout. It would be preferable to do the reverse, position the callout on translation, but I'm not aware of any methods to support that.

   We actually seem to be doing basically the same thing as Fluent. DOMLocalization uses MutationObserver to hear about the callout being added and needing translation, which is the same way we're hearing that it needs positioning. But in what order are the mutation callbacks executed? I guess that is why it's an intermittent issue. I'll ask around and see if there's a trick to that. But in the meantime, we can just force translate the callout before positioning.

3. The top-end positioner reuses some code from the old (pre-bug 1790819) top positioner.

4. The top-end positioner uses the top positioner's space properties to compute whether the callout fits in it, but the position sorting algorithm doesn't lend it those properties, so it goes a little haywire.

5. The top-end positioner (and other specialized positioners we might add in the future) shouldn't be used unless the message explicitly wants it. Bug 1790819 added some dynamic positioning logic to use when the callout doesn't fit in the message's desired position.

   Basically, it makes a list of all the positions in which the callout does fit, and then sorts them by the difference between available space and needed space. So instead of going down the list and picking the first position in which the callout fits, it picks the position that has the most extra space. If the callout doesn't fit in any of the positions, it falls back to the message's desired position, allowing it to overflow the viewport.

   So it's possible for the top-end position to be chosen for a message that isn't really suitable for it. I think it'd be good to add a property to that positioner reflecting that it shouldn't be chosen dynamically, and then we can reuse that property if/when we add more bespoke, message-specific positions.

Comment 1

[Shane Hughes [:aminomancer]]

NI Zibi - I'm guessing this doesn't exist, but wondering if there is some way to listen/wait for Fluent translation on a given element (or even on the root could work). We basically have a race condition with Fluent and our own childList MutationObserver callback. But what we do in that callback indirectly depends on Fluent, since it positions the element according to an algorithm that cares about the element's dimensions (which, in turn, depend on the strings that are rendered in the element). The consequences are mitigated by a couple requestAnimationFrame calls, but it still occasionally mispositions the element.

Here's what I'm currently thinking:

async function doPositionContainer(container) {
  if (container._translated) {
    // If the callout has already been translated, just position it.
    container.position();
  } else {
    // Synchronously translate strings in the callout because they influence
    // the size of the callout, which will affect position computation.
    document.l10n.pauseObserving();
    await document.l10n.translateElements(
      container.querySelectorAll("[data-l10n-id]")
    );
    requestAnimationFrame(() => container.position());
    document.l10n.resumeObserving();
    container._translated = true;
  }
  container.classList.remove("hidden");
}

Comment 2

[Shane Hughes [:aminomancer]]

Attachment: WIP: Bug 1795529 - Clean up Feature Callout positioning behavior.

Comment 3

[Negin]

Just to chime in for additional context/in case this might cover anything you were looking at, but this patch that should land soon covers the case for the tab pickup container getting collapsed and the feature callout originally not repositioning.

Comment 4

[Shane Hughes [:aminomancer]]

(In reply to Negin from comment #3)

Just to chime in for additional context/in case this might cover anything you were looking at, but this patch that should land soon covers the case for the tab pickup container getting collapsed and the feature callout originally not repositioning.

Thanks for fixing that bug!