Skip to content

Feat: Improve the precision of label bounding box calculations and rendering#3418

Open
fzaman-linz wants to merge 1 commit intocytoscape:unstablefrom
linz:feat/text-metrics
Open

Feat: Improve the precision of label bounding box calculations and rendering#3418
fzaman-linz wants to merge 1 commit intocytoscape:unstablefrom
linz:feat/text-metrics

Conversation

@fzaman-linz
Copy link
Copy Markdown

@fzaman-linz fzaman-linz commented Oct 14, 2025

This PR improves the precision of label bounding box calculations and rendering by introducing new style properties and refining label metrics handling.

  • text-metrics (default | actual)
    Allows to choose between browser-default text metrics or actual font metrics (actualBoundingBoxAscent / Descent) for more accurate label height calculations.

  • bbox-margin-error-x / bbox-margin-error-y
    Provides fine-grained control over bounding box expansion to compensate for browser rendering inaccuracies. These replace the previous hardcoded marginOfError.

  • Label Dimension Calculation Enhancements
    calculateLabelDimensions() now supports text-metrics: actual, using actualBoundingBoxAscent and actualBoundingBoxDescent for more precise vertical alignment and height.

Associated issues:

Checklist

Author:

  • The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
  • Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
  • The associated GitHub issues are included (above).
  • Notes have been included (above).
  • For new or updated API, the index.d.ts Typescript definition file has been appropriately updated.

Reviewers:

  • All automated checks are passing (green check next to latest commit).
  • At least one reviewer has signed off on the pull request.
  • For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

@fzaman-linz
Copy link
Copy Markdown
Author

cc: @maxkfranz @mikekucera

@fzaman-linz
Copy link
Copy Markdown
Author

fzaman-linz commented Feb 1, 2026

Hi guys, could you please have a look cc: @d2fong

cc: @maxkfranz @mikekucera

@mikekucera
Copy link
Copy Markdown
Contributor

Hi, thank you for your submission. Sorry for the long delay in response. Currently this repo isn't being actively maintained. The original maintainers (ie Max and me) have moved on to other things. I can only take a look at things when I have a bit of time here or there.

Its been a policy to not add new visual properties unless there's a real demand for them. The reason is that all visual properties are public API, and any new ones need to be maintained and documented forever. Personally I don't think we should add visual properties for fine adjustments to bounding box calculations. They will be incomprehensible to most users of the library and they will make it harder to make changes to the bounding box calculations in the future.

I quickly tested the changes on the demo page using label backgrounds. I set text-matricks: 'actual' and background-padding: 0, the result looks much better with this PR. There's less extra space at the top and bottom in this example:

Screenshot 2026-02-16 at 2 26 10 PM

Here are my suggestions for moving forward with this PR:

  • Remove the bbox-margin-error-x / bbox-margin-error-y properties, the default of 2 is fine. Its not worth it to add two VPs just for this.
  • Remove the text-metrics property and just make the new calculations the default. The new calculations seem to be better so just make them default. That way we can make more changes or revert if needed in the future.
  • Provide a test page or JS fiddle that makes it easy to test the changes.
  • Ask @d2fong to take a second look and do some testing as well.

@mikekucera mikekucera added pinned A long-lived issue, such as a discussion feedback Waiting for feedback on comments, or changes to a PR. labels Mar 18, 2026
@fzaman-linz fzaman-linz force-pushed the feat/text-metrics branch 2 times, most recently from aef5c55 to 6809168 Compare April 17, 2026 02:07
@fzaman-linz
Copy link
Copy Markdown
Author

Thanks for the feedback @mikekucera.
I've made the changes you suggested and here is a demo: https://codepen.io/fzaman-linz/pen/pvEYvxj
cc: @d2fong @maxkfranz

@maxkfranz
Copy link
Copy Markdown
Member

Screenshot 2026-02-16 at 2 26 10 PM

What happens if you have a label like 'apple' instead of just 'a'?

If you have several nodes like the above image, the text (baseline) on all the labels should line up. That's important.

Try these labels. They should all line up as long as the label positions are the same and the node dimensions are the same:

  • 'yes' (descender)
  • 'no' (no ascender nor descender)
  • 'not' (ascender)

@fzaman-linz
Copy link
Copy Markdown
Author

fzaman-linz commented Apr 21, 2026

@maxkfranz Yes, label text baselines won't line up across the board. Tight bounding boxes need per‑word heights (which shifts baselines), while consistent baselines need fixed heights (which reintroduces the gaps we're trying to avoid). That's why keeping text-metrics: default | actual makes sense so it leaves the choice with the user.
Are you ok with that?
image

@maxkfranz
Copy link
Copy Markdown
Member

@fzaman-linz, the team will discuss this on a call on Thursday 23 April.

@maxkfranz
Copy link
Copy Markdown
Member

@fzaman-linz, in the meantime, would you please update the OP with the current status of the feature (i.e. only currently proposed props)?

@fzaman-linz
Copy link
Copy Markdown
Author

@maxkfranz I've updated the OP and also updated the example URL to showcase default vs actual.
https://codepen.io/fzaman-linz/pen/pvEYvxj
image

@maxkfranz
Copy link
Copy Markdown
Member

Great. Thanks

@maxkfranz
Copy link
Copy Markdown
Member

The team is discussing this feature and the details of the properties etc., and we'll have an update some time next week.

@maxkfranz maxkfranz linked an issue Apr 27, 2026 that may be closed by this pull request
4 tasks
@maxkfranz maxkfranz requested review from maxkfranz May 4, 2026 14:58
let halfBorderWidth = borderWidth / 2;
let padding = ele.pstyle( 'text-background-padding' ).pfValue;
let marginOfError = 2; // expand to work around browser dimension inaccuracies
let marginOfErrorX = ele.pstyle( 'bbox-margin-error-x' ).pfValue; // expand to work around browser dimension inaccuracies
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are larger values than 2 needed for specific cases?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From @mikekucera :

Remove the bbox-margin-error-x / bbox-margin-error-y properties, the default of 2 is fine. Its not worth it to add two VPs just for this.

Comment thread src/style/properties.mjs
valign: { enums: [ 'top', 'center', 'bottom' ] },
halign: { enums: [ 'left', 'center', 'right' ] },
justification: { enums: [ 'left', 'center', 'right', 'auto' ] },
textMetrics: { enums: [ 'default', 'actual' ] },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These values should be more descriptive so they're obvious as to what they do. E.g. 'aligned' | 'tight'. Or whatever names would be clear.


let labelDims = this.calculateLabelDimensions( ele, text );
let lineHeight = ele.pstyle('line-height').pfValue;
let size = ele.pstyle('font-size').pfValue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you upload some screenshots of some line-height:1 multiline labels? We should verify that this change does not cause regressions. There are likely edge cases that normPerLineHeight was handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feedback Waiting for feedback on comments, or changes to a PR. pinned A long-lived issue, such as a discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve the accuracy of label bounding box calculations and rendering

3 participants