Repository navigation
Conversation
0f5cf06 to
4891dc1
Compare
|
Yes, I've updated this PR to support Android as well. |
|
Thanks @koji-1009, waiting for it. |
There was a problem hiding this comment.
Code Review
This pull request introduces AddressBarController to enable mobile browser address bar collapse for Flutter web apps by making the body scrollable and handling touch inputs via Touch Events. It also updates FullPageDimensionsProvider and EngineFlutterView to correctly track viewport changes on mobile. Feedback on these changes includes reusing the existing PointerDataConverter to prevent pointer ID collisions, using CSS calc() for the spacer height to support device rotation, replacing invalid for-in loops over DomTouchList with standard for loops to fix compilation errors, and explicitly specifying the touch device kind during pointer data conversion.
fc27ace to
ceed65d
Compare
|
Hey @koji-1009! This has not entered the review queue yet since it is a draft, are you still planning to move forward with this change? :) |
|
Ah ok! Thanks for letting me know! |
ceed65d to
a3983e9
Compare
a3983e9 to
38c6852
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces AddressBarController to enable mobile browser address bar collapse for Flutter web apps on Android and iOS by setting up scroll machinery and translating touch events into pointer data. Feedback on these changes suggests restoring modified global styles on <body> and <html> during dispose, replacing unsafe null assertions on domWindow.innerHeight with null-coalescing fallbacks, using Timer.run instead of Timer(Duration.zero, ...), and refactoring Map.forEach to a for-in loop to align with Effective Dart guidelines.
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks a lot for this, @koji-1009, and for the detailed write-up and videos. It's useful confirmation that pan-y plus That said, I don't think we want to take this direction in the engine. Because Flutter keeps scrolling while the browser The direction we're exploring instead is letting the browser own the page scroll, with Flutter mirroring it. There's an |
|
Thanks for looking at this, @flutter-zl. This was a PoC, and leaving out an opt-in flag and any change to Tap-to-top isn't handled here, as you say. But it doesn't work on current stable either, where the address bar never collapses, so I think it can be tracked and handled separately. On iOS native it is a separate path from touch input too: the engine detects the status bar tap with an offscreen One reason I made this an engine PR rather than a package was that the iOS address bar height wasn't reflected in the view size, which is the code that now conflicts with #193744. If the engine gets the height right, this becomes much easier to ship as an external package. A package can also take fixes to JS-side handling quickly, without waiting for an SDK release. I've also tried the PoC you linked, and the approach looks good. That said, it seems to show the issue I struggled with most while making this PR: the frame rate drops while the address bar hides on iOS and Android. Here I kept the view size fixed during the bar animation and reported the covered strip as padding instead, so the app isn't relaid out while the bar moves. Also, since the document height there follows the page content, a page that fits on screen seems to leave nothing to scroll, so the bar doesn't appear to collapse on it. Here the spacer doesn't depend on the content, so the bar behaves the same on every page. I hope these are useful as a reference. To sum up, I'm fine with this direction living in an external package, as long as the sizing is handled in the engine; tap-to-top can be tracked on its own. This PR was meant as a PoC to explore the approach, so I'm happy to close it 👍 |
Thanks for looking into this. I am closing this PR. |
Note
This is a proof-of-concept for discussion, not a merge-ready PR.
Looking for feedback on the approach before investing further.
An opt-in flag, and any change to the existing input path (
PointerBinding), are out of scope here.Summary
Addresses #69529
On mobile the browser address bar never collapses because Flutter web sets
touch-action: noneon body, so the browser never sees a scroll. Four things, on Android and iOS, and only when Flutter owns the whole page:touch-action: pan-yon body, so the browser reads a vertical drag as a scroll.<html>, giving the document real overflow — a scroll for the browser to consume and spend on its address bar.pointerrawupdate, becausepan-ymakes the browser cancel the touch Pointer Events mid-drag.iOS Safari
ios.mov
Android Chrome (Secure context)
android.mp4
Design decisions
touch-action: pan-yon mobile.pan-ythe touch Pointer Event stream ends mid-drag, so it cannot carry touch input at all.pointerTypeand tilt, and leave multi-touch movement on the throttledtouchmove.Details
Touch input
Touch-type Pointer Events are withheld from
PointerBindingand Touch Events are translated intoPointerDatain their place. The withholding happens in the bubble phase on the view root viastopImmediatePropagation— the controller is constructed beforePointerBinding, so its listener runs first. It is deliberately not a capture listener on the window: platform views and native text fields are descendants of the view root and see the events before anything is stopped.pointerrawupdate(MDN, Pointer Events Level 3) is the more precise movement source, since the browser throttlestouchmovewhile it is scrolling. Safari doesn't implement it and Chrome withholds it outside a secure context, sotouchmoveis the fallback.Two things follow from the spec. It never fires for the changes that fire
pointerdown/pointerup, so the contact's lifetime comes from Touch Events. And it carries no id that can be matched to a Touch contact, so it is used only while a single finger is down; with more than one, thetouchmoves, which do carry ids, drive the movement.scroll-snap
Android uses
y proximitywith acalc(100lvh + 1px)spacer: Chrome won't collapse the bar undermandatory, and a longer scroll range would keep scrolling the page after the browser chrome is already gone.On iOS the snapping is not about the bar but about the scroll range.
y mandatoryparks the page on a snap target in the middle of a 10000px spacer, thousands of pixels from either end, which keeps pull-to-refresh and rubber-banding out of reach and stops Safari's momentum scrolling from carrying on past the gesture. Underproximityboth come back.Test app (lib/main.dart)
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.