Skip to content

fix: unselect selected comment on scroll#15463

Open
printfdebugging wants to merge 3 commits intomainfrom
private/printfdebugging/comment-fix
Open

fix: unselect selected comment on scroll#15463
printfdebugging wants to merge 3 commits intomainfrom
private/printfdebugging/comment-fix

Conversation

@printfdebugging
Copy link
Copy Markdown
Contributor

Change-Id: I617603782260d4c3c19e8bdeafc794e3b7457d1d

  • Resolves: #
  • Target version: main

Summary

buggy comments

  • click on a comment in writer
  • start scrolling
  • the comment gets stuck and doesn't go off the screen
before.mp4

fix

  • comment is automatically unselected when scrolling
after.mp4

@caolanm caolanm force-pushed the private/printfdebugging/comment-fix branch from 0046885 to 4126ab9 Compare April 13, 2026 15:22
@printfdebugging printfdebugging force-pushed the private/printfdebugging/comment-fix branch from 4126ab9 to d17e06e Compare April 14, 2026 08:21
@mistmist
Copy link
Copy Markdown
Contributor

is this related to the changes?

  1 failing

  1) Annotation Tests
       Visibility at Different Window Widths (increasing):
     AssertionError: Timed out retrying after 10000ms: expected '<div#comment-container-1.cool-annotation.cool-annotation-collapsed-show>' to be 'visible'

This element `<div#comment-container-1.cool-annotation.cool-annotation-collapsed-show>` is not visible because its ancestor has `position: fixed` CSS property and it is overflowed by other elements. How about scrolling to the element with `cy.scrollIntoView()`?
      at Context.eval (integration_tests/desktop/writer/annotation_spec.js:150:44)

@printfdebugging
Copy link
Copy Markdown
Contributor Author

is this related to the changes?

  1 failing

  1) Annotation Tests
       Visibility at Different Window Widths (increasing):
     AssertionError: Timed out retrying after 10000ms: expected '<div#comment-container-1.cool-annotation.cool-annotation-collapsed-show>' to be 'visible'

This element `<div#comment-container-1.cool-annotation.cool-annotation-collapsed-show>` is not visible because its ancestor has `position: fixed` CSS property and it is overflowed by other elements. How about scrolling to the element with `cy.scrollIntoView()`?
      at Context.eval (integration_tests/desktop/writer/annotation_spec.js:150:44)

probably, let me check.

Signed-off-by: Sahil Gautam <sahil.gautam@collabora.com>
Change-Id: I617603782260d4c3c19e8bdeafc794e3b7457d1d
When scrolling, the selected comment should get unselected.

Signed-off-by: Sahil Gautam <sahil.gautam@collabora.com>
Change-Id: Id2c290fc67856c80cc5d9c359885ead3090fee6c
@printfdebugging printfdebugging force-pushed the private/printfdebugging/comment-fix branch from d17e06e to d3c6928 Compare April 15, 2026 08:43
@printfdebugging
Copy link
Copy Markdown
Contributor Author

the cypress test doesn't fail locally on latest main, so rebased the PR to see if it passes on the CI, if not, i will see what's wrong.

Comment thread browser/src/canvas/sections/CommentListSection.ts Outdated
Copy link
Copy Markdown
Contributor

@eszkadev eszkadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good to me

Signed-off-by: Sahil Gautam <sahil.gautam@collabora.com>
Change-Id: I77d46bf7669b9ae81df8ced13092c3dd0b612445
@printfdebugging printfdebugging force-pushed the private/printfdebugging/comment-fix branch from d3c6928 to eaf925a Compare April 15, 2026 12:07
@printfdebugging printfdebugging enabled auto-merge (rebase) April 15, 2026 13:50
@eszkadev
Copy link
Copy Markdown
Contributor

weird multi-page behavior:
Kooha-2026-04-15-16-03-06.webm

maybe we could implement the method for it too?

@eszkadev
Copy link
Copy Markdown
Contributor

it is more complicated

@printfdebugging
Copy link
Copy Markdown
Contributor Author

weird multi-page behavior: Kooha-2026-04-15-16-03-06.webm

maybe we could implement the method for it too?

can we do that in a followup? would be good to have this fix out atleast.

@eszkadev
Copy link
Copy Markdown
Contributor

weird multi-page behavior: Kooha-2026-04-15-16-03-06.webm
maybe we could implement the method for it too?

can we do that in a followup? would be good to have this fix out atleast.

Agree on that

Copy link
Copy Markdown
Contributor

@eszkadev eszkadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it breaks "scroll to comment" functionality causing random jumps currently (if comment target is not on a screen)

we need to fix that first, before merge (to not exchange bug for bug)

@github-project-automation github-project-automation Bot moved this from To Review to In Progress in Collabora Online Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants