allenai / pdf-component-library

51 stars 5 forks source link

Fix Scroll Issue in Semantic Reader #135

Closed huytr1995 closed 2 years ago

huytr1995 commented 2 years ago

Description

Ref: https://github.com/allenai/scholar/issues/32270

The fix for scrolling is not working in the Semantic Reader. The recursive algorithm did do the job but the parentElement that it ended up yielding is BODY which is satisfy the condition we checked for scrollHeight > clientHeight but not doing anything. There is also other edge cases we didn't consider.

Reviewer Instructions

Credit goes to this link: https://developpaper.com/js-determines-whether-the-element-can-be-scrolled/

The person who wrote this block was mentioned on how the check for scrollable that we are using is 100% correct but there is deviation. And the correct solution is using scrollTop verify that. Verified that this solution works in Semantic Reader. I did a yarn build and copy the pdf-components.js into the node_module of S2 and it works as intended.

Testing Plan

Manually test by clicking all the option of TOC and expect them to be able to scroll to the correct position. Other than that lint and format.

Output / Screenshots

Pdf-Component https://user-images.githubusercontent.com/84343285/175996038-9272c210-75d0-43f0-8fb8-022e72036a33.mov

Semantic Reader

https://user-images.githubusercontent.com/84343285/176559381-02eb1c2b-d85b-4a82-9003-9edb9a65b437.mov

New Recording to verify Prod works with checking for overflow hidden

https://user-images.githubusercontent.com/84343285/176732009-77a4ded1-88ce-4135-a933-880437ce9a9a.mov

A11y

No A11y involvement in this

pauls-ai2 commented 2 years ago

My favorite part of code you linked to was this line:

  if (!el instanceof HTMLElement) {
    console.log("fuck off");
    return false;
  }

tenor-240553179