duckduckgo / iOS

DuckDuckGo iOS Application
https://itunes.apple.com/us/app/duckduckgo-privacy-browser/id663592361?mt=8
Apache License 2.0
1.82k stars 413 forks source link

Scrolling causes the WebView to resize and lose previous page Snapshot #730

Closed cornedor closed 5 months ago

cornedor commented 3 years ago

Some sites do not work very well with these resizes, they will resize elements based of the height of the webview, wich causes laggy resizes of those elements while scrolling. See the examples below, one is in DuckDuckGo, the other in Safari

RPReplay-Final1602279407 RPReplay-Final1602279563

brindy commented 3 years ago

Thanks for this. Is it something you'd like to fix as part of Hacktoberfest maybe?

For anyone else looking at this, please note that fix will need to be very focused. An significant change to the UI or UX will not be accepted. Likewise, no changes to copy will be accepted either.

Thanks in advance to whoever picks this up!

iAmNaz commented 3 years ago

@brindy

This issue was interesting to look at since I experienced a similar one before. The issue was caused by the content view being stretched when the bars are hidden. I tried some fix while I am not sure about the UX you are expecting. Here is a recording of it.

In order to make it look like nothing much has changed from a UX experience I think we need to synch the scroll offset with the animation of the navigation bar.

https://user-images.githubusercontent.com/9004747/111905721-122c2380-8a88-11eb-8368-ee97875c1b86.mp4

brindy commented 3 years ago

Hi @iAmNaz - it appears no one responded to you. Apologies for that and thanks for commenting.

Does your fix also fix the problem with the previous page preview disappearing? Steps:

  1. Go to a page
  2. Click on a link
  3. Scroll to hide the scroll bars
  4. Start swiping back

Expect: Screen should show previous page

Actual: Screen is blank

Thanks!

iAmNaz commented 3 years ago

@brindy

The previous page is there when swiping back. I can still see subtle zooming and it seems to happen when my mouse cursor is above an image.

https://user-images.githubusercontent.com/9004747/124354904-0cc2a580-dc41-11eb-81c9-a20cbba6b69f.mp4

brindy commented 3 years ago

Thanks for the video.

In that video you don’t scroll to hide the bars after the navigation, but before swiping back. Does the previous page still appear if you do?

The final piece is that the scroll position of the previous page is also lost to test this.

  1. Load a site
  2. Scroll down
  3. Click a link
  4. After the navigation a roll to hide the bars
  5. Swipe back

Expected: Previous page should be visible while swiping and the previous page should stay the scroll position.

Do you have the code somewhere so I can try it?

Thanks again!

iAmNaz commented 3 years ago

Would be better if you could try this https://github.com/iAmNaz/iOS/tree/fix-webview-scroll-resizing.

brindy commented 3 years ago

Thanks for sharing. Your branch is massively out of date so it's not going to merge, but just to confirm it looks like the only line of significance is this one:

https://github.com/duckduckgo/iOS/pull/919/files#diff-23f45a546b44732f442ecaff49aa3d7bbdb120eadd9cf312fc61b8d5181b6ad0R361

I can try that against the main build and will let you know. Thanks!

cornedor commented 3 years ago

Fyi, the NOS site has a new design where the header no longer resizes based on the viewport height. I've build a more simple test case: https://corne.vercel.app/height

nazmariano commented 3 years ago

@cornedor if possible, could you add an image in that box to mimic the NOS sample? I think it is not very obvious with a solid color if the bounds changed. Thanks

cornedor commented 3 years ago

I've added an image background with roughly the same styling

brenoxp commented 3 years ago

I was able to find a solution that works well for this website: https://corne.vercel.app/height Basically, I updated the top and bottom containerView constraints to safeArea.top and superview.bottom respectively, and then I control the scrollView.contentInset based on the visibility of the omniBar and the toolbar. But when I tested on websites like https://www.w3schools.com/css/css_image_gallery.asp the fixed bar on top gets behind the omniBar which makes me think that the solution is to resize the content of the scrollView and manage the contentInset at the same time. Testing https://corne.vercel.app/height on Safari shows that the content size changes but the image keeps its size. @cornedor is it possible to add a fixed element on top and bottom of the page so that I can test?

cornedor commented 3 years ago

@brenoxp I've added two fixed elements to the page. I've also clarified what the pixel values are, and added the height of the container. Now it is more clear that only the window height changes, but not the height of the container element.

gneil90 commented 3 years ago

@brindy I am eager to help with this issue, is the status of the ticket up-to-date and still help wanted?

brindy commented 5 months ago

fwiw the OP's problem was fixed. Thanks and sorry for not keeping this up to date.