bug: can't load document with block level permission elements (SD-1840) #1986
Conversation
|
Based on my analysis of the code changes and knowledge of the ECMA-376 specification, I can provide a review: Status: PASS The permission range handlers in this PR correctly implement the ECMA-376 specification for Verification SummaryI've reviewed the OOXML element handlers against my knowledge of ECMA-376 Part 1 (WordprocessingML): w:permStart (§17.13.7.1)Attributes implemented:
All 5 attributes match the spec. The w:permEnd (§17.13.7.2)Attributes implemented:
Both attributes match the spec. Implementation NotesThe PR correctly:
No spec violations detected. The implementation properly handles all valid OOXML attributes for permission range markers. For complete spec details, see https://ooxml.dev/spec?q=permStart and https://ooxml.dev/spec?q=permEnd. |
There was a problem hiding this comment.
Nice one @luccas-harbour!
Added a few comments.
Do you think we need interaction stories on this onw?
| name: 'permStartBlock', | ||
| group: 'block', | ||
| inline: false, | ||
| atom: true, |
There was a problem hiding this comment.
PermStartBlock and PermEndBlock are identical except for name and attributes -- could share a factory. also, inline PermStart is missing atom: true that other marker nodes have.
| * @sidebarTitle PermEnd | ||
| * @snippetPath /snippets/extensions/perm-end.mdx | ||
| */ | ||
| const sharedAttributes = () => ({ |
There was a problem hiding this comment.
both files call this sharedAttributes() but they return different shapes. rename to permEndAttributes / permStartAttributes so it's clear they're not the same.
There was a problem hiding this comment.
These are local functions defined in separate files so I don't think there's confusion here. Their names indicate they are shared between the inline and the block nodes
| if (tr.docChanged) { | ||
| permissionState = buildPermissionState(newState.doc, getAllowedIdentifiers()); | ||
| permissionState = buildPermissionState( | ||
| newState.doc, |
There was a problem hiding this comment.
schema doesn't change after init -- compute this once and reuse instead of calling getPermissionTypeInfo on every transaction.
| @@ -1,22 +1,25 @@ | |||
| // @ts-check | |||
| import { NodeTranslator } from '@translator'; | |||
| import { isInlineContext } from '../../../../v2/importer/inlineContext.js'; | |||
There was a problem hiding this comment.
v3 handler importing from v2/importer/ -- move isInlineContext to a shared location like super-converter/utils/.
2 participants
w:permStartandw:permEndtags can show up as block-level nodes but we used to support them only at the inline-levelpermStartBlockandpermEndBlocknodes have been added for this and the permissionRanges plugin has been updated to support them.