• Michael Goderbauer's avatar
    Don't trigger an assert when markNeedsSemanticsUpdate is called multiple times... · b551f534
    Michael Goderbauer authored
    Don't trigger an assert when markNeedsSemanticsUpdate is called multiple times in edge cases (#11544)
    
    * Don't trigger assert if a render object ceases to be a semantic boundary
    
    This bug was exposed by https://github.com/flutter/flutter/pull/11309, which caused the following assertion to trigger when scrolling in the Animation demo:
    
    ```
    The following assertion was thrown during _updateSemantics():
    'package:flutter/src/rendering/object.dart': Failed assertion: line 2626 pos 16: 'fragment is
    _InterestingSemanticsFragment': is not true.
    ```
    
    A minimal reproduction of the bug can be found in `semantics_10_test.dart`, which has been added as a regression test for the bug by this PR.
    
    Looking at that test, here is a description of the faulty behaviour:
    1. During the second `pumpWidget` call `RenderExcludeSemantics` marks itself as needing a semantics update (due to excluding going from `false` -> `true`).
    2. This causes the nearest ancestor with semantics information (here: `RenderSemanticsAnnotations` representing the "container" Semantics widget) to be added to the `_nodesNeedingSemantics` list.
    3. `RenderSliverList` (implementation behind ListView) marks itself as needing a semantics update (due to its changing children).
    4. This causes the `RenderSemanticsGestureHandler` to be added to the `_nodesNeedingSemantics` list.
    5. Next, canDrag is updated from `true` -> `false`. This means, `RenderSemanticsGestureHandler` is no longer a semantics boundary, it marks itself as needing a semantics update.
    6. The nearest ancestor with semantics (`RenderSemanticsAnnotations`, the "container") is added to the `_nodesNeedingSemantics` list (this is a no-op because it is already in the list).
    7. During `flushSemantics`, the `_nodesNeedingSemantics` list is walked. The first entry (`RenderSemanticsAnnotations`) updates the semantics tree to only contain the container widget and drop everything else (= no children of the ExcludeSemantics widget are walked).
    8. The second entry (`RenderSemanticsGestureHandler`) is updated. It does not add any semantics of its own and is no longer a semantics boundary. Therefore, it wants to merge its descendent semantics into its parents. Here is where the assert throws because the algorithm assumes that every entry in the `_nodesNeedingSemantics` list will produce and own an `_InterestingSemanticsFragment` (passing your semantics on to your parents is not interesting).
    
    The problem here seems to be step 4 in combination with step 5. In step 4 we rely on the fact that `RenderSemanticsGestureHandler` is an (explicit or implicit) semantics boundary and that it will be able to absorb the semantics change of `RenderSliverList`. This is true
    at this time. However, in step 4 `RenderSemanticsGestureHandler` decides to no longer be an (explicit or implicit) semantics boundary and our assumption from step 5 becomes incorrect. We did nothing to correct this assumption.
    
    This PR removes a node, that could potentially cease to be a (explicit or implicit) semantics boundary from the `_nodesNeedingSemantics` list to fix that problem. Please node that this does not mean that the node's semantics will not be updated: The node's closest ances
    tor with semantics is added to that list during the `markNeedsSemanticsUpdate` call. During `flushSemantics` we will walk from this node to update the semantics of it's children (if changed), which will include the node in question.
    
    * tiny fix
    
    * simplify test
    
    * analyzer fixes
    
    * review comments
    b551f534
smoke_test.dart 7.17 KB