fix editable placement in a11y mode #14581
Conversation
| /// | ||
| /// The position will be updated everytime Flutter Framework sends | ||
| /// 'TextInput.setEditableSizeAndTransform' message. | ||
| void updateElementPosition(_GeometricInfo geometricInfo); |
There was a problem hiding this comment.
Why are we renaming these?
In the last 6 months we renamed this specific method and it's input 3 times.
My comment applies here and everywhere in the PR, can you please explain the reasoning behind the new names?
There was a problem hiding this comment.
Because we are not only positioning the element but also sizing it. I decided that "placement" has less bias than "position", which in CSS and in Flutter only means the coordinate of the top-left corner and does not have anything to do with the size. Other possible bias-free alternatives are "layout" and "geometry", e.g. updateElementLayout or updateElementGeometry.
yjbanov
force-pushed
the
a11y-text-field-offset
branch
from
9a97c3b to
f9d7752
Compare
| /// enabled. With semantics enabled the placement is provided by the semantics | ||
| /// tree. | ||
| class GloballyPositionedTextEditingStrategy extends DefaultTextEditingStrategy { | ||
| GloballyPositionedTextEditingStrategy(HybridTextEditing owner) : super(owner); |
There was a problem hiding this comment.
now we are adding one more intermediate node to the class diagram.
I am curious why we did not just override placeElement (used to called positionlement) and remove @mustcall annotation from there.
Can you explain the reasoning preferably in the documentation so that next time another functionality comes we decide between creating one more intermediate node or just overriding the method?
There was a problem hiding this comment.
This class supplies functionality (global placement) that's relevant to its subclasses and not relevant to semantics, so I separated it out. There's no other reason. If we move this logic into DefaultTextEditingStrategy then the semantics implementation will depend on it but will have to undo it.
I prefer this approach because I find it easier to understand code that only adds missing functionality than understanding code the alters or undoes existing functionality.
For the purposes of this PR I'm happy either way, so LMK what you prefer.
There was a problem hiding this comment.
I think these already exist in the current code.
I think it is important to have a good class design and rely on solid patterns. When to do that mostly should depend on the project priorities. I believe the need purify this class has not emerged. Yet since you already wrote this code, let's get this one in.
|
Thanks Yegor! I left comment for the initial review. |
nturgut
requested review from
mdebbar
and removed request for
stuartmorgan-g
yjbanov
force-pushed
the
a11y-text-field-offset
branch
from
f9d7752 to
9b7660e
Compare
* 399a29e Roll src/third_party/dart 141fcfa61092..4681868aaf77 (9 commits) (flutter/engine#14590) * 97fd705 Roll src/third_party/skia 7e2dea568299..dcac29b02d02 (3 commits) (flutter/engine#14591) * 6d92887 Roll src/third_party/dart 4681868aaf77..b92fefdfe971 (1 commits) (flutter/engine#14592) * 4d7313f Roll fuchsia/sdk/core/mac-amd64 from f51Q_... to VC7eE... (flutter/engine#14593) * fac66c2 [web] Long press fix on Safari on IOS (flutter/engine#14588) * 32eb6e9 Roll src/third_party/skia dcac29b02d02..088913a63b7e (1 commits) (flutter/engine#14597) * ede1b6b Roll fuchsia/sdk/core/linux-amd64 from 25LzW... to TUoVa... (flutter/engine#14596) * cce5afd Roll src/third_party/dart b92fefdfe971..886615d0f95b (2 commits) (flutter/engine#14595) * 9b299f2 fix editable placement in a11y mode (flutter/engine#14581) * 3381392 [web] implement pushImageFilter (flutter/engine#14599)
3 participants
Separate
DefaultTextEditingStrategyinto globally positioning implementation (GloballyPositionedTextEditingStrategy) and semantics implementation (SemanticsTextEditingStrategy). The former globally positions the editables, while the latter uses the semantics tree for positioning.Fixes flutter/flutter#45199