search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.5k stars 245 forks source link

Scribe inserts a <BR> into empty inline elements #254

Open robinedman opened 10 years ago

robinedman commented 10 years ago

At the moment we insert a <BR> into empty elements. Is there a reason we do this for inline elements?

This was initially only done for <LI> and <P> elements, but in later commits it was expanded to include any empty element that is not a void element. I understand why we do this for block elements, and also why we don't for void elements. But when we do this for inline elements we cause line breaks to be rendered.

These line breaks have been causing issues in scribe-plugin-noting.

@OliverJAsh ? :)

(The code inserting the <BR> is on line 39 of ensure-selectable-containers.js)

OliverJAsh commented 10 years ago

Without the BR, you can't place the caret inside the element. I'm not sure if this would be a problem for inline elements (empty inline elements could just get stripped instead).

What happens when you create an inline element initially by clicking on the toolbar? It has no content, so without a BR, what would happen? You could give it a try.

Then again I've probably forgotten a million other things that could come into play here.

On 25 September 2014 12:20, Robin Edman notifications@github.com wrote:

At the moment we insert a
into empty elements. Is there a reason we do this for inline elements?

This was initially only done for

  • and

    elements, but in later commits it was expanded to include any empty element that is not a void element. I understand why we do this for block elements, and also why we don't for void elements. But when we do this for inline elements we cause line breaks to be rendered.

    These line breaks have been causing issues in scribe-plugin-noting https://github.com/guardian/scribe-plugin-noting.

    @OliverJAsh https://github.com/OliverJAsh ? :)

    (The code inserting the
    is on line 39 of ensure-selectable-containers.js https://github.com/guardian/scribe/blob/master/src/plugins/core/formatters/html/ensure-selectable-containers.js )

    — Reply to this email directly or view it on GitHub https://github.com/guardian/scribe/issues/254.

    • robinedman commented 10 years ago

    Thanks for the input Olly! :)

    For some reason it's tricky replicating this without modifying the HTML directly. This line isn't executing when I click on the toolbar (maybe browsers do something so the elements created using e.g. the bold command aren't truly empty).

    So as far as I can tell it wouldn't hurt to stop inserting this BR into inline elements. (I think it'd make more sense not to be able to put the caret inside of leftover empty inline elements as well).

    For the noting plugin we're using custom elements, which are inline by default. If we'd change this logic in ensure-selectable-containers.js I think that instead of using a whitelist of allowed elements (or extending the blacklist we currently use), checking whether the computed display property is inline might be a good approach.

    theefer commented 10 years ago

    Looking at the computed property isn't a bad idea, but it leaves you at the mercy of odd behaviour if a stylesheet overrides any of the styles. I wonder if you could use the browser to tell you the default display of a given tag though...

    Visit theguardian.com. On your mobile and tablet, download the Guardian iPhone and Android apps theguardian.com/guardianapp and our tablet editions theguardian.com/editions. Save up to 57% by subscribing to the Guardian and Observer - choose the papers you want and get full digital access. Visit subscribe.theguardian.com

    This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way. Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.

    Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP. Registered in England Number 908396

    robinedman commented 10 years ago

    You can in FF using getDeafultComputedStyle. Also, for some reason, it's not possible to get the values in Chrome at all when this function is executing (using getComputedStyle), as it seems the style values haven't been set yet. It works if I do it aftewrards in the web console.

    So if there's no real reason we're doing this for anything else than block elements I think the simplest solution would be to limit the BR insertion to only block elements using the list of block elements in scribe-common.

    robinedman commented 10 years ago

    Tried limiting this to block elements using that list. During the non-exhaustive testing I did I didn't notice any issues.

    robinedman commented 9 years ago

    Reopening as this wasn't fixed by #258