draft-convert icon indicating copy to clipboard operation
draft-convert copied to clipboard

Custom Inline style's HTML is produced inconsistently

Open shapiroronny opened this issue 7 years ago • 8 comments

Hey,

I've added a custom inline style for coloring some text. Using styleToHTML I create a <span> with some inline style. If the same text range also has 'underline' style there is no way to control the processing order of the styles and they are treated in the order they were applied. This may cause issues as <span style="color: blue"><u>text</u></span> and <u><span style="color: blue">text</span></u> are different. In the latter case the color style doesn't affect the underline.

I've tried changing the way underlines are treated to create a <span style="text-decoration: underline"> but unfortunately the spans don't merge even if they have the same range so I'm facing the same problem.

Is there a way around this ? I imagine merging identical tags (and the style attribute) is a possible solution. Another solution is somehow controlling the style processing order by somehow specifying priority.

A temporary workaround is to remove the underline before I apply a color and then set it again. If the underline is the last added style it's always handled first when the custom HTML is generated, but that feels really hacky.

Thanks

shapiroronny avatar Aug 21 '17 14:08 shapiroronny

I ran into this issue as well. A somewhat hacky workaround is to add code like this when you're outputting a color:

if (cssStyle.color) {
  cssStyle.textDecorationColor = cssStyle.color;
  cssStyle.textDecorationLine = 'inherit';
}

Won't work in all browsers, but it at least doesn't require you to know anything about which order the styles are applied. Also bloats the output some. :P

Would be awesome to have some sort of API-level support for this scenario in styleToHTML, though. Possibly an array of other styles that also apply to the current section under consideration, or a mechanism for assigning priority order for handling styles?

NatPri avatar Feb 03 '18 00:02 NatPri

Thank you for outlining the issues @shapiroronny and @NatPri. I've run into similar issues with ordering and I like both of your suggestions for building this into draft-convert.

I tend to prefer the idea of having styles be mergeable into a single tag, since I'd imagine having a prioritization order could get really tough to manage as the number of styles/tags increases (and it could possibly not shake out to be a single, consistent priority order). Additionally the fact that inline styles are rendered with a styleMap means that any inline styles must be represented in the editor UI as purely CSS, so the same merging is already happening in the editor. We could probably do a similar thing with tag names to keep some semantic meaning in the output.

I'm unsure of what an elegant and clear API would be for it. I think there's value to the API being a ReactElement for HTML output instead of a made-up data structure. We could still use a ReactElement and if it has no other attributes other than style merge with other styles. Would it be clear what's going on in that case, or if there's a mix of style-only elements and elements with other attributes (that would then cause the merge to be aborted)?

Interested to hear any thoughts. Thanks again!

benbriggs avatar May 08 '18 19:05 benbriggs

One case where I'm not sure that simple merging of styles would necessarily work would be in the following scenario (which I need to support as well):

<u>underline only 
  <span style="color: red">colored text</span> 
more text</u>

If I want to have the underline match the color of the text, I'd need to output a <u> tag instead of a <span> tag (or something along those lines).

My thinking around this is that currently the API doesn't expose enough context as input to styleToHtml for consumers to handle this case; the handler only sees that it needs to add a color style, and has no idea that there's an underline style affecting it as well, which would allow it to output <u style=...> instead of <span style=...>. It feels like there would be a lot of assumptions draft-convert would need to make if it were to try to handle this case outside of the styleToHtml handler.

That said, I do think that merging non-conflicting styles into fewer tags makes sense from the standpoint of making the output cleaner.

NatPri avatar May 08 '18 21:05 NatPri

Definitely - that was kind of what I was getting at with:

We could probably do a similar thing with tag names to keep some semantic meaning in the output

I think we could do something to look at tag names where it inherits the new tag name as it merges until it merges in one with meaning (i.e. something other than span). If it encounters more than one non-span tag name it could either start a new stack of merged styles or just abort merging altogether. Do you think that'd cover your use case?

benbriggs avatar May 08 '18 22:05 benbriggs

If I understand your proposal correctly, the idea is basically extend the notion of style maps from draft-js to include tags, without a change to the public styleToHtml API? So in this scenario specifically, with underline mapped to u and the color mapped to a color style attached to a non-semantic span, draft-convert would know that the 'current' semantic tag is u and if it gets back a span from styleToHtml, then it knows to continue to use u as the tag to attach styles to instead of the span. Thus the output would become:

<u>before <u style="color: red">color</u> after</u>

I'm trying to figure out how it might handle mixing other tags in, like b/strong or i/em, like for something like <u>before <b>bold <span style="color: red">color</span></b> after</u>; it sounds like the span would become b in this case, which wouldn't override the underline color as expected (unless it was hard-coded that u is special somehow).

Or is the idea to change the API to present this sort of "merged" view of the world to the styleToHtml, and allow it to make the final decision on what to output?

NatPri avatar May 08 '18 23:05 NatPri

Or is the idea to layer all the tags together when there is a "conflicting' semantic tag, e.g:

<u>before </u><u><b>bold </b></u><u style="color:red"><b>color</b></u><u> after</u>

NatPri avatar May 08 '18 23:05 NatPri

Yep that first paragraph was pretty much what I was thinking. I hadn't thought about nesting the same semantic tag though - you're right it wouldn't get the underline color otherwise. I wonder if those sorts of cases will end up just needing to be covered by being able to see all currently-applied styles like you mentioned while applying one. Some context on why that doesn't currently exist is because this was built in parallel with draft-extend where styleToHTML is accumulated together from plugins representing disjoint features.

benbriggs avatar May 09 '18 18:05 benbriggs

Yeah, that's kinda why I was leaning towards providing additional context to styleToHtml (or potentially some other API) and letting clients handle this explicitly, for example, by providing a list of all of the styles that apply to the section of text under consideration (perhaps as a separate, additional parameter?), and allowing it to output the appropriate element and styles all-up. Or possibly even providing some sort of post-processing step that essentially exposes the constructed DOM somehow (though at that point it might be better to say that client code should handle any such post-processing, e.g. using the DOMParser API or any other similar HTML manipulation libraries). I can't think of a good heuristic for dealing with this in draft-convert that doesn't make incorrect assumptions or that doesn't involve hard-coding a workaround for the underline + text color scenario, neither of which seems like a great idea.

I don't use draft-extend, nor do I have much familiarity with the internals of draft-convert, so I don't have any particular perspective there, but I can see how draft-extend in particular might make API changes more difficult to swallow.

NatPri avatar May 09 '18 23:05 NatPri