Closed Bug 828931 Opened 12 years ago Closed 2 years ago

HTML+CSS editing rules for inline styles

Categories

(Core :: DOM: Editor, defect, P5)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1803044

People

(Reporter: glazou, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

new |-U 8| patch and test
(deleted), patch
Details | Diff | Splinter Review
The HTML+CSS editing rules for inline styles in the editor are horked. Steps to reproduce the bug: 1. open the Midas Demo at http://www-archive.mozilla.org/editor/midasdemo/ 2. enter some text in the editable area 3. select some of the text you just entered 4. click on the B button and then on the I button Expected result, verified in FF12: one span only with two inline styles Actual result in Nightly 21.0a1 2013-01-09: two nested spans with one inline style each.. This could be a rather old regression. Impacts Thunderbird for rich-mail composition, BlueGriffon, BlueGriffon EPUB, SeaMonkey.
Regression from bug 746515, apparently
For the following test: data:text/html,<!doctype html><div contenteditable><span style="font-style:italic">bar</span></div><script>getSelection().selectAllChildren(document.body.firstChild);document.execCommand("stylewithcss", false, true);document.execCommand("bold");document.body.textContent = document.body.firstChild.innerHTML;</script> WebKit replies: <span style="font-style:italic"><span style="font-weight: bold;">bar</span></span> This seems suboptimal to me. Before the fix for bug 746515, Gecko used to reply: <span style="font-style:italic; font-weight: bold;">bar</span> We now reply: <span style="font-weight: bold;"><span style="font-style:italic">bar</span></span> which is as ugly as WebKit's answer but different :-) What we did in the past was cleaner, simpler and more in line with all the magic in nsHTMLEditorStyle.cpp.
Found one of the reasons why Aryeh's code is not working, but it's not the only one: in http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditorStyle.cpp#317 , the code |element->GetAttrCount() != 1| is wrong and makes the editor behave badly... Aryeh forgot the mozdirty attribute so this should be checking attrCount != 1 and (attrCount !=2 || (no_mozdirty_attribute_on_element))
Attached patch patch part 1 (obsolete) (deleted) — — Splinter Review
Attached patch fix (obsolete) (deleted) — — Splinter Review
Better fix. Does exactly what is described above.
Attachment #700430 - Attachment is obsolete: true
Can you please generate a patch with 8 lines of context and send this off to try? (We also need a testcase for this patch, of course.)
Blocks: 746515
Attached patch new |-U 8| patch and test (deleted) — — Splinter Review
Ehsan, I have no hg account. Two options here: you vouch for me and get me one so I can send to try server and ultimately check that in; you do it yourself ;-) I think it's highly time for me to regain write access in Gecko.
Attachment #700493 - Attachment is obsolete: true
(In reply to comment #7) > Created attachment 700943 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=700943&action=edit > new |-U 8| patch and test > > Ehsan, I have no hg account. Two options here: you vouch for me and get me > one so I can send to try server and ultimately check that in; you do it > yourself ;-) > I think it's highly time for me to regain write access in Gecko. I'll definitely vouch for you! Just file a bug and CC me on it, please.
(In reply to Daniel Glazman (:glazou) from comment #0) > The HTML+CSS editing rules for inline styles in the editor are horked. > Steps to reproduce the bug: > > 1. open the Midas Demo at http://www-archive.mozilla.org/editor/midasdemo/ > 2. enter some text in the editable area > 3. select some of the text you just entered > 4. click on the B button and then on the I button > > Expected result, verified in FF12: one span only with two inline styles > > Actual result in Nightly 21.0a1 2013-01-09: two nested spans with one inline > style each.. This is by design, for consistency between HTML and CSS modes. It's bug 746515 patch part 5, "Only add style="" to empty <span>s". Once the bold adds a span, it's not empty, so italics will add a new span instead of modifying the existing one. See bug 746515 comment 1 for rationale: > The spec says to only ever add style="" to empty spans. This also makes CSS > and non-CSS mode more closely analogous -- every time non-CSS mode would add > an element, so does CSS mode. This is more likely to prevent discrepancies. > E.g., consider > > data:text/html,<!doctype html> > <div contenteditable><font color=red>foo</font><br><font > color=red>bar</font></div> > <script> > getSelection().selectAllChildren(document.body.firstChild.firstChild); > document.execCommand("stylewithcss", false, true); > document.execCommand("underline"); > getSelection().selectAllChildren(document.body.firstChild.childNodes[2]); > document.execCommand("stylewithcss", false, false); > document.execCommand("underline"); > getSelection().removeAllRanges(); > </script> > > In Gecko, "foo"'s underline is red, while "bar"'s is black. This is because > CSS mode adds text-decoration: underline to the <font>, while non-CSS mode > nests it in a <u>, and the color of the underline is the color of the > element that generates it. > > So my patch here is going to make Gecko only ever add style="" to an empty > span. If the current node isn't an empty span (after removing styles), it > will get wrapped. Assuming we do want to keep the spec's behavior, it might be worthwhile to add a test for this case.

Bulk-downgrade of unassigned, >=3 years untouched DOM/Storage bug's priority.

If you have reason to believe this is wrong, please write a comment and ni :jstutte.

Severity: major → S4
Priority: -- → P5

Fixed in bug 1803044.

Status: NEW → RESOLVED
Closed: 2 years ago
Duplicate of bug: 1803044
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: