From 08214525810c4feb860eec80efc2e62357219142 Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Mon, 25 Aug 2025 15:28:46 -0500 Subject: [PATCH 1/9] Refactor branch graph to support keyboard focus and events The branch graph previously supported mouse hover and click events to show the tooltip and open a commit in a new tab, respectively. The graph did not support keyboard focus or keypress events to manage these interactions. Add keyboard focus, blur, and keypress custom event listeners. Update each commit link to have a unique accessible name. Refactor circle helper methods to avoid duplication in code. Changelog: fixed --- .../javascripts/network/branch_graph.js | 60 ++++++++++++++----- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 80a1d27510171d..2d2b0b91b167ed 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -246,24 +246,54 @@ export default class BranchGraph { appendAnchor(x, y, commit) { const { r, top, options } = this; - r.circle(x, y, 10) - .attr({ + const circle = r.circle(x, y, 10); + + // .click(() => visitUrl(options.commit_url.replace('%s', commit.id), true)) + // .hover( + // function () { + // this.tooltip = r.commitTooltip(x + 5, y, commit); + // top.push(this.tooltip.insertBefore(this)); + // return this.tooltip.toFront(); + // }, + // function () { + // top.remove(this.tooltip); + // return this.tooltip && this.tooltip.remove() && delete this.tooltip; + // }, + // ); + + circle.attr({ fill: '#000', opacity: 0, cursor: 'pointer', - }) - .click(() => visitUrl(options.commit_url.replace('%s', commit.id), true)) - .hover( - function () { - this.tooltip = r.commitTooltip(x + 5, y, commit); - top.push(this.tooltip.insertBefore(this)); - return this.tooltip.toFront(); - }, - function () { - top.remove(this.tooltip); - return this.tooltip && this.tooltip.remove() && delete this.tooltip; - }, - ); + }); + circle.node.setAttribute('tabindex', '0'); + circle.node.addEventListener('focus', () => { + this.tooltip = r.commitTooltip(x + 5, y, commit); + top.push(this.tooltip.insertBefore(this.node)); + return this.tooltip.toFront(); + }); + circle.node.addEventListener('blur', () => { + top.remove(this.tooltip); + return this.tooltip && this.tooltip.remove() && delete this.tooltip; + }); + circle.node.addEventListener('keydown', (e) => { + const { key, keyCode } = e; + if (key === 'Enter' || keyCode === 13) { + visitUrl(options.commit_url.replace('%s', commit.id), true); + } + }); + circle.node.addEventListener('mouseover', () => { + this.tooltip = r.commitTooltip(x + 5, y, commit); + top.push(this.tooltip.insertBefore(this.node)); + return this.tooltip.toFront(); + }); + circle.node.addEventListener('mouseout', () => { + top.remove(this.tooltip); + return this.tooltip && this.tooltip.remove() && delete this.tooltip; + }); + circle.node.addEventListener('click', () => { + visitUrl(options.commit_url.replace('%s', commit.id), true); + }); } drawDot(x, y, commit) { -- GitLab From b33bdbdb432637c09f3ecc9377d5adcef4a007ef Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Mon, 25 Aug 2025 16:05:34 -0500 Subject: [PATCH 2/9] Refactor branch graph tooltip functions to DRY helpers --- .../javascripts/network/branch_graph.js | 94 +++++++++---------- 1 file changed, 45 insertions(+), 49 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 2d2b0b91b167ed..7f6d7185653b0f 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -1,4 +1,4 @@ -/* eslint-disable func-names, consistent-return */ +/* eslint-disable consistent-return */ import $ from 'jquery'; import axios from '~/lib/utils/axios_utils'; @@ -180,6 +180,11 @@ export default class BranchGraph { return $(element).scroll(() => this.renderPartialGraph()); } + removeTooltip() { + this.top.remove(this.tooltip); + return this.tooltip && this.tooltip.remove() && delete this.tooltip; + } + scrollDown() { this.element.scrollTop(this.element.scrollTop() + 50); return this.renderPartialGraph(); @@ -208,6 +213,22 @@ export default class BranchGraph { return this.element.scrollTop(0); } + /** + * Shows a tooltip for a commit at the specified position + * @param {Object} options - The tooltip configuration options + * @param {Object} options.r - The Raphael paper instance + * @param {number} options.x - The x-coordinate for tooltip positioning + * @param {number} options.y - The y-coordinate for tooltip positioning + * @param {Object} options.commit - The commit object containing commit data + * @returns {Object} The tooltip element brought to front + */ + showTooltip(options) { + const { r, x, y, commit } = options; + this.tooltip = r.commitTooltip(x + 5, y, commit); + this.top.push(this.tooltip.insertBefore(this.node)); + return this.tooltip.toFront(); + } + appendLabel(x, y, commit) { if (!commit.refs) { return; @@ -245,55 +266,30 @@ export default class BranchGraph { } appendAnchor(x, y, commit) { - const { r, top, options } = this; + const { r, options } = this; const circle = r.circle(x, y, 10); - - // .click(() => visitUrl(options.commit_url.replace('%s', commit.id), true)) - // .hover( - // function () { - // this.tooltip = r.commitTooltip(x + 5, y, commit); - // top.push(this.tooltip.insertBefore(this)); - // return this.tooltip.toFront(); - // }, - // function () { - // top.remove(this.tooltip); - // return this.tooltip && this.tooltip.remove() && delete this.tooltip; - // }, - // ); - - circle.attr({ - fill: '#000', - opacity: 0, - cursor: 'pointer', - }); - circle.node.setAttribute('tabindex', '0'); - circle.node.addEventListener('focus', () => { - this.tooltip = r.commitTooltip(x + 5, y, commit); - top.push(this.tooltip.insertBefore(this.node)); - return this.tooltip.toFront(); - }); - circle.node.addEventListener('blur', () => { - top.remove(this.tooltip); - return this.tooltip && this.tooltip.remove() && delete this.tooltip; - }); - circle.node.addEventListener('keydown', (e) => { - const { key, keyCode } = e; - if (key === 'Enter' || keyCode === 13) { - visitUrl(options.commit_url.replace('%s', commit.id), true); - } - }); - circle.node.addEventListener('mouseover', () => { - this.tooltip = r.commitTooltip(x + 5, y, commit); - top.push(this.tooltip.insertBefore(this.node)); - return this.tooltip.toFront(); - }); - circle.node.addEventListener('mouseout', () => { - top.remove(this.tooltip); - return this.tooltip && this.tooltip.remove() && delete this.tooltip; - }); - circle.node.addEventListener('click', () => { - visitUrl(options.commit_url.replace('%s', commit.id), true); - }); + + circle.attr({ + fill: '#000', + opacity: 0, + cursor: 'pointer', + }); + + // Keyboard event handlers + circle.node.setAttribute('tabindex', '0'); + circle.node.addEventListener('focus', () => this.showTooltip({ r, x, y, commit })); + circle.node.addEventListener('blur', () => this.removeTooltip()); + circle.node.addEventListener('keydown', (e) => { + const { key, keyCode } = e; + if (key === 'Enter' || keyCode === 13) { + visitUrl(options.commit_url.replace('%s', commit.id), true); + } + }); + + // Mouse event handlers + circle.node.addEventListener('mouseover', () => this.showTooltip({ r, x, y, commit })); + circle.node.addEventListener('mouseout', () => this.removeTooltip()); + circle.node.addEventListener('click', () => visitUrl(options.commit_url.replace('%s', commit.id), true)); } drawDot(x, y, commit) { -- GitLab From 07160c43f048601d723ba9ec8a115ee420ef8ed1 Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Mon, 25 Aug 2025 16:28:10 -0500 Subject: [PATCH 3/9] Add accessible link labels with i18n to branch graph --- app/assets/javascripts/network/branch_graph.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 7f6d7185653b0f..5477e006df06cb 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -268,6 +268,9 @@ export default class BranchGraph { appendAnchor(x, y, commit) { const { r, options } = this; const circle = r.circle(x, y, 10); + const accessibleLinkLabel = sprintf(__('Commit %{commitIdSlice}. Opens in a new window.'), { + commitIdSlice: commit.id.slice(0,8), + }); circle.attr({ fill: '#000', @@ -275,8 +278,12 @@ export default class BranchGraph { cursor: 'pointer', }); - // Keyboard event handlers + // Attributes required for keyboard and screen reader usability circle.node.setAttribute('tabindex', '0'); + circle.node.setAttribute('role', 'link'); + circle.node.setAttribute('aria-label', accessibleLinkLabel); + + // Keyboard event handlers circle.node.addEventListener('focus', () => this.showTooltip({ r, x, y, commit })); circle.node.addEventListener('blur', () => this.removeTooltip()); circle.node.addEventListener('keydown', (e) => { -- GitLab From a5178a140eca5dc94d7d668fdf4f8467d55f304c Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Mon, 25 Aug 2025 16:59:46 -0500 Subject: [PATCH 4/9] Improve performance rendering circles in branch graph --- .../javascripts/network/branch_graph.js | 58 +++++++++++++------ 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 5477e006df06cb..1290f627142906 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -268,9 +268,6 @@ export default class BranchGraph { appendAnchor(x, y, commit) { const { r, options } = this; const circle = r.circle(x, y, 10); - const accessibleLinkLabel = sprintf(__('Commit %{commitIdSlice}. Opens in a new window.'), { - commitIdSlice: commit.id.slice(0,8), - }); circle.attr({ fill: '#000', @@ -278,25 +275,48 @@ export default class BranchGraph { cursor: 'pointer', }); - // Attributes required for keyboard and screen reader usability - circle.node.setAttribute('tabindex', '0'); - circle.node.setAttribute('role', 'link'); - circle.node.setAttribute('aria-label', accessibleLinkLabel); - - // Keyboard event handlers - circle.node.addEventListener('focus', () => this.showTooltip({ r, x, y, commit })); - circle.node.addEventListener('blur', () => this.removeTooltip()); - circle.node.addEventListener('keydown', (e) => { - const { key, keyCode } = e; - if (key === 'Enter' || keyCode === 13) { + // Raphael JS won't render if I destructure the node + // eslint-disable-next-line prefer-destructuring + const node = circle.node; + node.setAttribute('tabindex', '0'); + node.setAttribute('role', 'link'); + node.setAttribute('aria-label', sprintf(__('Commit %{commitIdSlice}. Opens in a new window.'), { + commitIdSlice: commit.id.slice(0, 8), + })); + + // Create a single unified event handler instead of multiple functions + // This reduces memory overhead from multiple function closures + const handleInteraction = (e) => { + const { type, key, keyCode } = e; + + switch (type) { + case 'focus': + case 'mouseover': + this.showTooltip({ r, x, y, commit }); + break; + case 'blur': + case 'mouseout': + this.removeTooltip(); + break; + case 'keydown': + if (key === 'Enter' || keyCode === 13) { + visitUrl(options.commit_url.replace('%s', commit.id), true); + } + break; + case 'click': visitUrl(options.commit_url.replace('%s', commit.id), true); + break; + default: + break; } - }); + }; - // Mouse event handlers - circle.node.addEventListener('mouseover', () => this.showTooltip({ r, x, y, commit })); - circle.node.addEventListener('mouseout', () => this.removeTooltip()); - circle.node.addEventListener('click', () => visitUrl(options.commit_url.replace('%s', commit.id), true)); + // Add all event listeners using the same handler function + // This is more memory efficient than creating separate handler functions + const events = ['focus', 'blur', 'keydown', 'mouseover', 'mouseout', 'click']; + for (let i = 0; i < events.length; i += 1) { + node.addEventListener(events[i], handleInteraction, { passive: events[i] !== 'keydown' }); + } } drawDot(x, y, commit) { -- GitLab From 6e0900a75d4a68e4162c3cee3dd93d4924574aca Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Wed, 27 Aug 2025 11:27:24 -0500 Subject: [PATCH 5/9] Refactor event listeners and add cleanup function to branch graph --- .../javascripts/network/branch_graph.js | 52 ++++++++++++++++++- .../pages/projects/network/network.js | 17 +++++- .../pages/projects/network/show/index.js | 47 ++++++++++++++--- 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 1290f627142906..bd25e636be5553 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -27,6 +27,9 @@ export default class BranchGraph { this.unitSpace = 10; this.prev_start = -1; this.load(); + // New stuff + this.scrollHandler = this.handleScroll.bind(this); + this.isDestroyed = false; } load() { @@ -174,10 +177,55 @@ export default class BranchGraph { } } + handleScroll() { + if (this.isDestroyed) return; + + // Throttle scroll events + if (this.scrollTimeout) return; + + this.scrollTimeout = setTimeout(() => { + this.renderPartialGraph(); + this.scrollTimeout = null; + }, 16); // ~60 frames per second. TODO: check this. + } + bindEvents() { - const { element } = this; + if (this.isDestroyed) return; + + // Use throttled scroll handler + this.element.on('scroll', this.scrollHandler); + + // Return cleanup function + return () => this.unbindEvents(); + } - return $(element).scroll(() => this.renderPartialGraph()); + unbindEvents() { + if (this.element) { + this.element.off('scroll', this.scrollHandler); + } + + if (this.scrollTimeout) { + clearTimeout(this.scrollTimeout); + this.scrollTimeout = null; + } + } + + destroy() { + this.isDestroyed = true; + this.unbindEvents(); + + // Clean up Raphael + if (this.r) { + this.r.remove(); + this.r = null; + } + + // Clean up data structures + this.preparedCommits = null; + this.parents = null; + this.commits = null; + this.element = null; + this.options = null; } removeTooltip() { diff --git a/app/assets/javascripts/pages/projects/network/network.js b/app/assets/javascripts/pages/projects/network/network.js index 10fa77267ce866..5e271cc62aa5e2 100644 --- a/app/assets/javascripts/pages/projects/network/network.js +++ b/app/assets/javascripts/pages/projects/network/network.js @@ -11,11 +11,24 @@ export default class Network { this.filter_ref.click(() => this.submit()); this.branch_graph = new BranchGraph(this.network_graph, this.opts); this.network_graph.css({ height: `${vph}px` }); - $('body').css({ 'overflow-y': 'hidden' }); - $('.content-wrapper').css({ 'padding-bottom': 0 }); + // Store cleanup function + this.cleanupBranchGraph = this.branch_graph.bindEvents(); + } submit() { return this.filter_ref.closest('form').submit(); } + + // destroy() { + // if (this.cleanupBranchGraph) { + // this.cleanupBranchGraph(); + // } + // if (this.branch_graph) { + // this.branch_graph.destroy(); + // } + // // Reset body styles + // $('body').css({ 'overflow-y': 'hidden' }); + // $('.content-wrapper').css({ 'padding-bottom': 0 }); + // } } diff --git a/app/assets/javascripts/pages/projects/network/show/index.js b/app/assets/javascripts/pages/projects/network/show/index.js index 58b703bdfdab6a..0112cacc4c4cc1 100644 --- a/app/assets/javascripts/pages/projects/network/show/index.js +++ b/app/assets/javascripts/pages/projects/network/show/index.js @@ -36,14 +36,45 @@ const initRefSwitcher = () => { initRefSwitcher(); (() => { - if (!$('.network-graph').length) return; + // if (!$('.network-graph').length) return; - const networkGraph = new Network({ - url: $('.network-graph').attr('data-url'), - commit_url: $('.network-graph').attr('data-commit-url'), - ref: $('.network-graph').attr('data-ref'), - commit_id: $('.network-graph').attr('data-commit-id'), - }); + // const networkGraph = new Network({ + // url: $('.network-graph').attr('data-url'), + // commit_url: $('.network-graph').attr('data-commit-url'), + // ref: $('.network-graph').attr('data-ref'), + // commit_id: $('.network-graph').attr('data-commit-id'), + // }); + + // addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); + + const initNetworkGraph = () => { + if (!$('.network-graph').length) return null; + + const networkGraph = new Network({ + url: $('.network-graph').attr('data-url'), + commit_url: $('.network-graph').attr('data-commit-url'), + ref: $('.network-graph').attr('data-ref'), + commit_id: $('.network-graph').attr('data-commit-id'), + }); + + addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); + + // Return cleanup function + return () => { + if (networkGraph) { + networkGraph.destroy(); + } + }; + }; + + initNetworkGraph(); + // Initialize and store cleanup + // const cleanupNetworkGraph = initNetworkGraph(); - addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); + // Add cleanup listener + // window.addEventListener('beforeunload', () => { + // if (cleanupNetworkGraph) { + // cleanupNetworkGraph(); + // } + // }); })(); -- GitLab From 089b3b9c8497fdb7425aa7a1b05a6cb853d0b2d8 Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Wed, 27 Aug 2025 12:30:15 -0500 Subject: [PATCH 6/9] Add cleanup function to branch graph and network --- .../javascripts/network/branch_graph.js | 16 ++++------ .../pages/projects/network/network.js | 29 +++++++++---------- .../pages/projects/network/show/index.js | 27 ++++------------- 3 files changed, 26 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index bd25e636be5553..de3bf857a1e739 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -16,6 +16,7 @@ export default class BranchGraph { this.scrollLeft = this.scrollLeft.bind(this); this.scrollUp = this.scrollUp.bind(this); this.scrollDown = this.scrollDown.bind(this); + this.scrollHandler = this.handleScroll.bind(this); this.preparedCommits = {}; this.mtime = 0; this.mspace = 0; @@ -26,10 +27,8 @@ export default class BranchGraph { this.unitTime = 30; this.unitSpace = 10; this.prev_start = -1; - this.load(); - // New stuff - this.scrollHandler = this.handleScroll.bind(this); this.isDestroyed = false; + this.load(); } load() { @@ -183,19 +182,16 @@ export default class BranchGraph { // Throttle scroll events if (this.scrollTimeout) return; + // Tested with _.debounce, but the redraw was not as smooth or consistent this.scrollTimeout = setTimeout(() => { this.renderPartialGraph(); this.scrollTimeout = null; - }, 16); // ~60 frames per second. TODO: check this. + }, 16); // 1000 milliseconds / 60 frames = 16.66... milliseconds per frame } bindEvents() { if (this.isDestroyed) return; - - // Use throttled scroll handler this.element.on('scroll', this.scrollHandler); - - // Return cleanup function return () => this.unbindEvents(); } @@ -214,13 +210,13 @@ export default class BranchGraph { this.isDestroyed = true; this.unbindEvents(); - // Clean up Raphael + // Clean up Raphael for GC if (this.r) { this.r.remove(); this.r = null; } - // Clean up data structures + // Clean up data structures for GC this.preparedCommits = null; this.parents = null; this.commits = null; diff --git a/app/assets/javascripts/pages/projects/network/network.js b/app/assets/javascripts/pages/projects/network/network.js index 5e271cc62aa5e2..1cd785cae6dfc5 100644 --- a/app/assets/javascripts/pages/projects/network/network.js +++ b/app/assets/javascripts/pages/projects/network/network.js @@ -8,27 +8,26 @@ export default class Network { this.opts = opts; this.filter_ref = $('#filter_ref'); this.network_graph = $('.network-graph'); + this.network_graph.css({ height: `${vph}px` }); this.filter_ref.click(() => this.submit()); this.branch_graph = new BranchGraph(this.network_graph, this.opts); - this.network_graph.css({ height: `${vph}px` }); - // Store cleanup function - this.cleanupBranchGraph = this.branch_graph.bindEvents(); - + this.resetBodyStyles(); + } + + // eslint-disable-next-line class-methods-use-this + resetBodyStyles() { + $('body').css({ 'overflow-y': 'hidden' }); + $('.content-wrapper').css({ 'padding-bottom': 0 }); } submit() { return this.filter_ref.closest('form').submit(); } - // destroy() { - // if (this.cleanupBranchGraph) { - // this.cleanupBranchGraph(); - // } - // if (this.branch_graph) { - // this.branch_graph.destroy(); - // } - // // Reset body styles - // $('body').css({ 'overflow-y': 'hidden' }); - // $('.content-wrapper').css({ 'padding-bottom': 0 }); - // } + destroy() { + if (this.branch_graph) { + this.branch_graph.destroy(); + this.resetBodyStyles(); + } + } } diff --git a/app/assets/javascripts/pages/projects/network/show/index.js b/app/assets/javascripts/pages/projects/network/show/index.js index 0112cacc4c4cc1..5f857fd672289b 100644 --- a/app/assets/javascripts/pages/projects/network/show/index.js +++ b/app/assets/javascripts/pages/projects/network/show/index.js @@ -36,17 +36,6 @@ const initRefSwitcher = () => { initRefSwitcher(); (() => { - // if (!$('.network-graph').length) return; - - // const networkGraph = new Network({ - // url: $('.network-graph').attr('data-url'), - // commit_url: $('.network-graph').attr('data-commit-url'), - // ref: $('.network-graph').attr('data-ref'), - // commit_id: $('.network-graph').attr('data-commit-id'), - // }); - - // addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); - const initNetworkGraph = () => { if (!$('.network-graph').length) return null; @@ -59,7 +48,6 @@ initRefSwitcher(); addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); - // Return cleanup function return () => { if (networkGraph) { networkGraph.destroy(); @@ -67,14 +55,11 @@ initRefSwitcher(); }; }; - initNetworkGraph(); - // Initialize and store cleanup - // const cleanupNetworkGraph = initNetworkGraph(); + const cleanupNetworkGraph = initNetworkGraph(); - // Add cleanup listener - // window.addEventListener('beforeunload', () => { - // if (cleanupNetworkGraph) { - // cleanupNetworkGraph(); - // } - // }); + window.addEventListener('beforeunload', () => { + if (cleanupNetworkGraph) { + cleanupNetworkGraph(); + } + }); })(); -- GitLab From 30ad4cf1ca1ae8eb539d3639452695a1b6506d6d Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Wed, 27 Aug 2025 16:22:25 -0500 Subject: [PATCH 7/9] Rewrite branch graph SR-only text to include author and message Previously the branch graph was announcing the avatars as a separate graphic. When the focus management was added to links, the reading order got jumbled up for screen readers. Now avatars are hidden from screen readers, and links include the commit message, author name, and instructions to open in new window. Co-authored-by: GitLab Duo Co-authored by @ms.mondrian --- .../javascripts/network/branch_graph.js | 37 ++++++------------- locale/gitlab.pot | 6 +-- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index de3bf857a1e739..2ec555e7e52c48 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -209,13 +209,13 @@ export default class BranchGraph { destroy() { this.isDestroyed = true; this.unbindEvents(); - + // Clean up Raphael for GC if (this.r) { this.r.remove(); this.r = null; } - + // Clean up data structures for GC this.preparedCommits = null; this.parents = null; @@ -319,20 +319,22 @@ export default class BranchGraph { cursor: 'pointer', }); - // Raphael JS won't render if I destructure the node - // eslint-disable-next-line prefer-destructuring - const node = circle.node; + const { node } = circle; node.setAttribute('tabindex', '0'); node.setAttribute('role', 'link'); - node.setAttribute('aria-label', sprintf(__('Commit %{commitIdSlice}. Opens in a new window.'), { - commitIdSlice: commit.id.slice(0, 8), - })); + node.setAttribute( + 'aria-label', + sprintf(__('%{commitMessage}, by %{authorName}. Opens in a new window.'), { + commitMessage: commit.message.split('\n', 1)[0], + authorName: commit.author.name || commit.authorName, + }), + ); // Create a single unified event handler instead of multiple functions // This reduces memory overhead from multiple function closures const handleInteraction = (e) => { const { type, key, keyCode } = e; - + switch (type) { case 'focus': case 'mouseover': @@ -372,28 +374,13 @@ export default class BranchGraph { const avatarBoxX = this.offsetX + this.unitSpace * this.mspace + 10; const avatarBoxY = y - 10; - const commitAuthorName = sprintf(__('Authored by %{authorName}'), { - authorName: commit.author.name, - }); - const image = r.image(commit.author.icon, avatarBoxX, avatarBoxY, 20, 20); + r.image(commit.author.icon, avatarBoxX, avatarBoxY, 20, 20); r.rect(avatarBoxX, avatarBoxY, 20, 20).attr({ stroke: this.colors[commit.space], 'stroke-width': 2, }); - /** - * Must set a constant when making multiple node() calls - * @see https://svgwg.org/svg2-draft/struct.html#TermCoreAttribute - * - * Tested and verified working in the following screen readers: - * MacOS - Safari, Chrome + VoiceOver - * Win11 - Chrome + NVDA, JAWS - */ - image.node.setAttribute('aria-label', commitAuthorName); - - image.node.setAttribute('role', 'img'); - return r .text(this.offsetX + this.unitSpace * this.mspace + 40, y, commit.message.split('\n')[0]) .attr({ diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b002d721a5106f..a6ac48e76d8695 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -702,6 +702,9 @@ msgstr "" msgid "%{codeStart}$%{codeEnd} will be treated as the start of a reference to another variable." msgstr "" +msgid "%{commitMessage}, by %{authorName}. Opens in a new window." +msgstr "" + msgid "%{commit_author_link} authored %{commit_authored_timeago}" msgstr "" @@ -9562,9 +9565,6 @@ msgstr "" msgid "Authored %{timeago} by %{author}" msgstr "" -msgid "Authored by %{authorName}" -msgstr "" - msgid "Authorization code:" msgstr "" -- GitLab From 9dd4fe4ce5392a77a0e855ee38457965ccb9dd2a Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Fri, 29 Aug 2025 09:14:06 -0500 Subject: [PATCH 8/9] Improve fault tolerance in branch graph keyboard usability Suggestions added from MR review by @ms.mondrian: * Include constants for ENTER, NUMPAD_ENTER in keydown listener * Properly destructure node from circle graphics * Remove reference to Raphael Paper element in tooltip method * Remove passive option from handleInteraction() event listeners --- app/assets/javascripts/network/branch_graph.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/network/branch_graph.js b/app/assets/javascripts/network/branch_graph.js index 2ec555e7e52c48..cb693bfdc8eeef 100644 --- a/app/assets/javascripts/network/branch_graph.js +++ b/app/assets/javascripts/network/branch_graph.js @@ -3,6 +3,7 @@ import $ from 'jquery'; import axios from '~/lib/utils/axios_utils'; import { visitUrl } from '~/lib/utils/url_utility'; +import { ENTER_KEY, NUMPAD_ENTER_KEY } from '~/lib/utils/keys'; import { __, sprintf } from '~/locale'; import Raphael from './raphael'; @@ -260,15 +261,14 @@ export default class BranchGraph { /** * Shows a tooltip for a commit at the specified position * @param {Object} options - The tooltip configuration options - * @param {Object} options.r - The Raphael paper instance * @param {number} options.x - The x-coordinate for tooltip positioning * @param {number} options.y - The y-coordinate for tooltip positioning * @param {Object} options.commit - The commit object containing commit data * @returns {Object} The tooltip element brought to front */ showTooltip(options) { - const { r, x, y, commit } = options; - this.tooltip = r.commitTooltip(x + 5, y, commit); + const { x, y, commit } = options; + this.tooltip = this.r.commitTooltip(x + 5, y, commit); this.top.push(this.tooltip.insertBefore(this.node)); return this.tooltip.toFront(); } @@ -319,7 +319,7 @@ export default class BranchGraph { cursor: 'pointer', }); - const { node } = circle; + const { node } = circle; node.setAttribute('tabindex', '0'); node.setAttribute('role', 'link'); node.setAttribute( @@ -334,18 +334,19 @@ export default class BranchGraph { // This reduces memory overhead from multiple function closures const handleInteraction = (e) => { const { type, key, keyCode } = e; + const normalizedEnterKey = ENTER_KEY || NUMPAD_ENTER_KEY; switch (type) { case 'focus': case 'mouseover': - this.showTooltip({ r, x, y, commit }); + this.showTooltip({ x, y, commit }); break; case 'blur': case 'mouseout': this.removeTooltip(); break; case 'keydown': - if (key === 'Enter' || keyCode === 13) { + if (key === normalizedEnterKey || keyCode === 13) { visitUrl(options.commit_url.replace('%s', commit.id), true); } break; @@ -361,7 +362,7 @@ export default class BranchGraph { // This is more memory efficient than creating separate handler functions const events = ['focus', 'blur', 'keydown', 'mouseover', 'mouseout', 'click']; for (let i = 0; i < events.length; i += 1) { - node.addEventListener(events[i], handleInteraction, { passive: events[i] !== 'keydown' }); + node.addEventListener(events[i], handleInteraction); } } -- GitLab From b93441da71294d57d62418e6a385913afc07741c Mon Sep 17 00:00:00 2001 From: Trevor Pierce Date: Wed, 3 Sep 2025 09:28:56 -0500 Subject: [PATCH 9/9] Refactor graph setup and teardown to discrete functions Co-authored by @psjakubowska --- .../pages/projects/network/show/index.js | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/pages/projects/network/show/index.js b/app/assets/javascripts/pages/projects/network/show/index.js index 5f857fd672289b..e40a0e52e902f8 100644 --- a/app/assets/javascripts/pages/projects/network/show/index.js +++ b/app/assets/javascripts/pages/projects/network/show/index.js @@ -36,10 +36,12 @@ const initRefSwitcher = () => { initRefSwitcher(); (() => { + let networkGraph = null; + const initNetworkGraph = () => { - if (!$('.network-graph').length) return null; + if (!$('.network-graph').length) return; - const networkGraph = new Network({ + networkGraph = new Network({ url: $('.network-graph').attr('data-url'), commit_url: $('.network-graph').attr('data-commit-url'), ref: $('.network-graph').attr('data-ref'), @@ -47,19 +49,16 @@ initRefSwitcher(); }); addShortcutsExtension(ShortcutsNetwork, networkGraph.branch_graph); + }; - return () => { - if (networkGraph) { - networkGraph.destroy(); - } - }; + const cleanupNetworkGraph = () => { + if (networkGraph) { + networkGraph.destroy(); + networkGraph = null; + } }; - const cleanupNetworkGraph = initNetworkGraph(); + initNetworkGraph(); - window.addEventListener('beforeunload', () => { - if (cleanupNetworkGraph) { - cleanupNetworkGraph(); - } - }); + window.addEventListener('beforeunload', cleanupNetworkGraph); })(); -- GitLab