Closed Bug 1113238 Opened 5 years ago Closed 5 years ago

whitespace: pre; doesn't copy correctly if only the nodes inside the range and not the parents have the style set on them

Categories

(Core :: Serializers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed

People

(Reporter: BenWa, Assigned: ehsan)

References

Details

Attachments

(3 files, 2 obsolete files)

With bug 116083 landed, copying from the cleopatra 'log' tab still doesn't work.

STR:
1) Open http://people.mozilla.org/~bgirard/cleopatra/#
2) Upload the attached file
3) Click on the 'Logging' tab in the middle of the page
4) Copy from the logging tab
Attached file 9fVRT8e2.dms (obsolete) —
Flags: needinfo?(ehsan.akhgari)
Attached file Testcase
What's happening is that in nsHTMLCopyEncoder::SetSelection, we only look at the parent chain starting from the selection's common ancestor.  In cases like this, or in Cleopatra, the individual nodes in the selection range are white-space: pre, but the parents are not, so the check we have in place fails.

I'm not quite sure how to fix this properly yet.  This whole code is a giant mess.  :(

Of course, I can definitely hack around this by traversing all of the nodes in the selection and see if there is any non-preformatted nodes in there, but that would be a gross hack which will not solve the bigger problem of handling selections with a mix of preformatted and normal nodes.
Attachment #8538621 - Attachment is obsolete: true
Flags: needinfo?(ehsan.akhgari)
BenWa, in the mean time, to work around this bug in Cleopatra, you can add |white-space: pre| to .logWidget.
Summary: whitespace: pre; copy doesn't work on more complex cases (cleopatra) → whitespace: pre; doesn't copy correctly if only the nodes inside the range and not the parents have the style set on them
Trying out a possible fix: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47161f183a89
Assignee: nobody → ehsan.akhgari
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> Trying out a possible fix:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47161f183a89

Real try link: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e152fe175770.  lots of test failures.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f671b65a7c5d is finally green.  Ignore the Android m-16 oranges, I verified separately that they pass on the most recent m-c.
This code is super-hairy, but I think this is the minimum amount of changes
that we need.

nsPlainTextSerializer::IsInPre() before this patch is completely broken, and
I changed it to maintain a stack of bools representing whether the elements
that we saw as we were traversing the tree are preformatted or not.

nsXHTMLContentSerializer maintains this information using a counter, which is
broken in case pre and non-preformatted elements are stacked underneath each
other, but I'm not sure why this code is using a counter and I didn't want to
change it drastically, so for now I'm just making it look at the element's
style first as opposed to its tag name.

Follow-up work may include exploring whether nsXHTMLContentSerializer should
use a stack similar to nsPlainTextSerializer, and also audit this code for
more places where things are hardcoded based on tag names where we should be
really looking at the style.
Attachment #8540218 - Flags: review?(bzbarsky)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> BenWa, in the mean time, to work around this bug in Cleopatra, you can add
> |white-space: pre| to .logWidget.

Workaround doesn't work because I run into bug 1119503.
Comment on attachment 8540218 [details] [diff] [review]
Make our plaintext and HTML serializers aware of CSS preformatted styles

>+  return mPreformatStack.empty() ? false : mPreformatStack.top();

I vaguely prefer

  return !mPreformatStack.empty() && mPreformatStack.top();

but either way.

>+    nsComputedDOMStyle::GetStyleContextForElementNoFlush(aElement, nullptr,

(Two places).

So when serializing stuff without frames this could get pretty expensive...  Can we just skip doing this for elements with no frames or something?  Are there cases in which we care about the preformatted state of those?

I guess if we're not flushing we could have weird transient behavior, but we could do a single flush up front or something, maybe.

r=me, but this part worries me.
Attachment #8540218 - Flags: review?(bzbarsky) → review+
This patch ensures that we check ShouldMaintainPreLevel() before attempting
to modify or read mPreLevel in order to avoid wasting time to compute
mPreLevel for elements without frames needlessly.  Computing this value for
such elements can incur expensive style calculations.
Attachment #8547629 - Flags: review?(bzbarsky)
This ensures that calls to nsComputedDOMStyle::GetStyleContextForElementNoFlush()
in the part 1 of this series give us the correct results.
Attachment #8547655 - Flags: review?(bzbarsky)
Comment on attachment 8547629 [details] [diff] [review]
Part 2: Only maintain the pre level status which can be potentially expensive if we may end up using it

r=me, assuming that we can't optimize away things in nsPlainTextSerializer.  If we can, a separate patch for that would be great.
Attachment #8547629 - Flags: review?(bzbarsky) → review+
Comment on attachment 8547655 [details] [diff] [review]
Part 3: Flush the styles before determining whether an element is preformatted

Can we do the flush only in the cases when !mRaw and whatnot?

r=me with that
Attachment #8547655 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8547629 [details] [diff] [review]
> Part 2: Only maintain the pre level status which can be potentially
> expensive if we may end up using it
> 
> r=me, assuming that we can't optimize away things in nsPlainTextSerializer. 
> If we can, a separate patch for that would be great.

I think that is only used for copying selections, no?  If that's true, it should not be affected by this issue.

(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8547655 [details] [diff] [review]
> Part 3: Flush the styles before determining whether an element is
> preformatted
> 
> Can we do the flush only in the cases when !mRaw and whatnot?
> 
> r=me with that

Yes, good idea!  Will do upon landing.
Flags: needinfo?(bzbarsky)
> I think that is only used for copying selections, no? 

Also used for "save as text" for web pages, probably.  Not that I expect anyone ever really uses that...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #15)
> > I think that is only used for copying selections, no? 
> 
> Also used for "save as text" for web pages, probably.  Not that I expect
> anyone ever really uses that...

OK.  Do you still want me to look into optimizing the nsPlainTextSerializer side of things?
Please?  If it's hard, or not going to help in the save as text case, let's not worry about it, but if we can do it easily, I'd say we should.
Sigh...  Hopefully I'll get to look at this again some day, chances are that it won't happen soon.
Flags: needinfo?(ehsan)
Comment on attachment 8547655 [details] [diff] [review]
Part 3: Flush the styles before determining whether an element is preformatted

The errors are caused because it's not possible to flush during editor operations.  I should have known better!  Confirmed with Boris on IRC that he's fine not taking part 3.
Attachment #8547655 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] from comment #18)
> Please?  If it's hard, or not going to help in the save as text case, let's
> not worry about it, but if we can do it easily, I'd say we should.

FWIW this cannot be done since IsInPre() is used unconditionally in nsPlainTextSerializer.
https://hg.mozilla.org/mozilla-central/rev/183898a49db1
https://hg.mozilla.org/mozilla-central/rev/139457679fc7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 116083
Depends on: 1123062
Depends on: 1125963
Blocks: 1119503
Comment on attachment 8540218 [details] [diff] [review]
Make our plaintext and HTML serializers aware of CSS preformatted styles

Approval Request Comment
[Feature/regressing bug #]: bug 1119503

[User impact if declined]: newlines missing when pasting pre-formatted text.

[Describe test coverage new/current, TreeHerder]: improved tests in this patch.

[Risks and why]: 

Ehsan said:

"The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."

[String/UUID change made/needed]: none
Attachment #8540218 - Flags: approval-mozilla-aurora?
Comment on attachment 8547629 [details] [diff] [review]
Part 2: Only maintain the pre level status which can be potentially expensive if we may end up using it

Approval Request Comment
[Feature/regressing bug #]: bug 1119503
[User impact if declined]: for this particular bug, wasted CPU cycles
[Describe test coverage new/current, TreeHerder]: no tests specific to this optimization
[Risks and why]: elsewhere, Ehsan said:

"The risk on this patch set is moderate.  We have tried to not change the existing behavior except for only the well known cases, but this code is really old and not very well understood.  I (ehsan) would like these fixes to be backported on Aurora as soon as we can to get broader testing, and will be on the hook for fixing any regressions found."
[String/UUID change made/needed]: none
Attachment #8547629 - Flags: approval-mozilla-aurora?
Comment on attachment 8540218 [details] [diff] [review]
Make our plaintext and HTML serializers aware of CSS preformatted styles

Prereq for bug 1119503. Note that although this is a large patch, the bulk of the patch is tests. Aurora+
Attachment #8540218 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8547629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1174452
Depends on: 1370737
You need to log in before you can comment on or make changes to this bug.