Page MenuHomeSoftware Heritage

assets/revision/diff-utils: Fix text selection in revision view
ClosedPublic

Authored by anlambert on Tue, Nov 17, 2:32 PM.

Details

Summary

A javascript click handler was modifying the view URL fragment without any checks.

Modifying the view URL fragment has the side effect of scrolling the view and prevents
proper text selection.

Surprisingly, that bug only occured on Firefox.

Test Plan

I tried to write a cypress test to reproduce the issue but simulating text
selection with the mouse does not seem to be possible with cypress.

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Tue, Nov 17, 2:32 PM

Build is green

Patch application report for D4491 (id=15932)

Rebasing onto 8a0f32d124...

Current branch diff-target is up to date.
Changes applied before test
commit afbb083eda6fba3358a6c9c393c58d464d3e4efc
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Tue Nov 17 14:26:32 2020 +0100

    assets/revision/diff-utils: Fix text selection in revision view
    
    Modifying view URL fragment without proper checks had the side effect of
    scrolling the view and thus preventing text selection.
    
    Surprisingly, that bug only occured on Firefox.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/471/ for more details.

Build is green

Patch application report for D4491 (id=15932)

Rebasing onto 8a0f32d124...

Current branch diff-target is up to date.
Changes applied before test
commit afbb083eda6fba3358a6c9c393c58d464d3e4efc
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Tue Nov 17 14:26:32 2020 +0100

    assets/revision/diff-utils: Fix text selection in revision view
    
    Modifying view URL fragment without proper checks had the side effect of
    scrolling the view and thus preventing text selection.
    
    Surprisingly, that bug only occured on Firefox.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/472/ for more details.

why store the current tab in a variable instead of just asking it when needed (using something similar to e.currentTarget.text.trim())?

why store the current tab in a variable instead of just asking it when needed (using something similar to e.currentTarget.text.trim())?

Because I do not like to duplicate code, the URL fragment modification happens in a function that is not only called by the click handler and it makes the code easier to read.

ardumont accepted this revision.Fri, Nov 20, 11:35 AM
This revision is now accepted and ready to land.Fri, Nov 20, 11:35 AM

Build is green

Patch application report for D4491 (id=16111)

Rebasing onto 57a0899490...

Current branch diff-target is up to date.
Changes applied before test
commit 6cc8f01cce1a5dc7c3692f8eb3129e35210d5249
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Tue Nov 17 14:26:32 2020 +0100

    assets/revision/diff-utils: Fix text selection in revision view
    
    Modifying view URL fragment without proper checks had the side effect of
    scrolling the view and thus preventing text selection.
    
    Surprisingly, that bug only occured on Firefox.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/484/ for more details.

Build is green

Patch application report for D4491 (id=16111)

Rebasing onto 6cc8f01cce...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-485-D4491.
Changes applied before test

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/485/ for more details.

Build is green

Patch application report for D4491 (id=16111)

Rebasing onto 6cc8f01cce...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-486-D4491.
Changes applied before test

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/486/ for more details.