The default bug view has changed. See this FAQ.

Invalidate affected frames when a range in a selection is modified

RESOLVED FIXED in mozilla12

Status

()

Core
Selection
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The following Range methods doesn't invalidate the selection properly:
SetStart
  SetStartBefore
  SetStartAfter
SetEnd
  SetEndBefore
  SetEndAfter
Collapse
SelectNode
SelectNodeContents
SurroundContents
Detach
(Assignee)

Comment 1

6 years ago
Created attachment 570516 [details] [diff] [review]
fix + tests, v1

Call InvalidateFrameSubtree() for all continuations of the
range common ancestor before/after the modification as needed.
Attachment #570516 - Flags: review?(Olli.Pettay)
Comment on attachment 570516 [details] [diff] [review]
fix + tests, v1


>+static void InvalidateAllFrames(nsINode* aNode)
>+{
>+  NS_PRECONDITION(aNode, "bad arg");
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
QI is a bit slow, and adds extra addref/release.

Could you use non-virtual nsINode::NodeType()
and switch-case.
DOCUMENT_NODE should cause invalidation to root frame, 
ATTRIBUTE_NODE, ENTITY_REFERENCE_NODE, ENTITY_NODE, NOTATION_NODE, DOCUMENT_FRAGMENT_NODE
should not cause any validation, and for the rest casting to nsIContent* should be safe.

> nsRange::SetStart(nsIDOMNode* aParent, PRInt32 aOffset)
> {
>   VALIDATE_ACCESS(aParent);
> 
>   nsCOMPtr<nsINode> parent = do_QueryInterface(aParent);
>+  AutoInvalidateSelection atEndOfBlock(this);
>   return SetStart(parent, aOffset);
> }

Could we just do the whole AutoInvalidateSelection thing in DoSetRange?

Does this affect badly to performance? IIRC we have some selection handling related
perf bugs open when one selects a huge table or so.

r- because I want QI of cycle collected objects reduced.
Attachment #570516 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 574179 [details] [diff] [review]
fix + tests, v2

> Could you use non-virtual nsINode::NodeType() and switch-case.

Fixed.  I think we only need to care about TEXT and ELEMENT (nsIContent)
and DOCUMENT (nsIDocument).  I don't think we can have frames for the
other ones(?)

> Could we just do the whole AutoInvalidateSelection thing in DoSetRange?

That would be overkill.  When removing/adding/changing nodes the frames are
already invalidated by other code.  Same for Selection methods which does
its invalidation in selectFrames().  Some methods also calls DoSetRange
multiple times.  Still, I agree it would be nice to consolidate the
invalidation part in one place.  I think a better way to do that though
is to implement the :selection pseudo-class and let the style system do
the updates.

> Does this affect badly to performance?

The performance impact should be negligible I think, I don't see how we can
fix this bug without some sort of frame invalidation code.

> IIRC we have some selection handling
> related perf bugs open when one selects a huge table or so.

Let me know if you have know the specific bug numbers and I'll test it -
I'll query Bugzilla to see what I can find.

(FYI, I'm in New Zealand time zone the next two weeks)
Attachment #570516 - Attachment is obsolete: true
Attachment #574179 - Flags: review?(bugs)
Comment on attachment 574179 [details] [diff] [review]
fix + tests, v2

 
>+  AutoInvalidateSelection atEndOfBlock(this);
Not having AutoInvalidateSelection in DoSetRange looks a bit error prone.
But I can see the performance reasons to not do that.

>+nsRange::GetRegisteredCommonAncestor()
>+{
>+  NS_ASSERTION(IsInSelection(),
>+               "GetRegisteredCommonAncestor only valid for range in selection");
>+  nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent);
>+  while (ancestor) {
>+    RangeHashTable* ranges =
>+      static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range));
>+    if (ranges->GetEntry(this)) {
>+      break;
>+    }
>+    ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent());
>+  }
This could be reasonable slow. We have still plenty of spare boolean flags in nsINode, so would it
make sense to add a flag if node may have a range in its RangeHashTable?
Though, actually, perhaps there should be flag to indicate if node has any properties.
Could you file a followup to investigate how often nodes have properties (I mean any property) and how often
GetProperty is used on nodes which don't have any properties.

>+  nsINode* GetRegisteredCommonAncestor();
Please document what this method does.

>+  struct NS_STACK_CLASS AutoInvalidateSelection {
>+    AutoInvalidateSelection(nsRange* aRange) : mRange(aRange) {
{ should be in the next line.


>+#ifdef DEBUG
>+      mWasInSelection = mRange->IsInSelection();
>+#endif
>+      if (!mRange->IsInSelection() || mIsNested)
>+        return;
if (expr) {
  stmt;
}
Attachment #574179 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
> >+nsRange::GetRegisteredCommonAncestor()
> >+{
> >+  NS_ASSERTION(IsInSelection(),
> >+               "GetRegisteredCommonAncestor only valid for range in selection");
> >+  nsINode* ancestor = GetNextRangeCommonAncestor(mStartParent);
> >+  while (ancestor) {
> >+    RangeHashTable* ranges =
> >+      static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range));
> >+    if (ranges->GetEntry(this)) {
> >+      break;
> >+    }
> >+    ancestor = GetNextRangeCommonAncestor(ancestor->GetNodeParent());
> >+  }
> This could be reasonable slow. We have still plenty of spare boolean flags
> in nsINode, so would it
> make sense to add a flag if node may have a range in its RangeHashTable?

That shouldn't be a problem here, GetNextRangeCommonAncestor returns the 
next ancestor with the CommonAncestorForRangeInSelection bit and if non-null
that ancestor MUST have this property with at least one range in the hash table.
It's rare to have more than one such ancestor (overlapping ranges).

> Though, actually, perhaps there should be flag to indicate if node has any
> properties.
> Could you file a followup to investigate how often nodes have properties (I
> mean any property) and how often
> GetProperty is used on nodes which don't have any properties.

Sure, that could be an optimization in general.
(Assignee)

Comment 6

5 years ago
Created attachment 584190 [details] [diff] [review]
part 2.  remove wallpaper for this bug in an earlier reftest
(Assignee)

Comment 7

5 years ago
With the nits fixed as suggested, and the wallpaper in an earlier reftest removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91909393c5a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2c62d75dea
Flags: in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/91909393c5a0
https://hg.mozilla.org/mozilla-central/rev/4b2c62d75dea
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 673108
(Assignee)

Updated

5 years ago
Blocks: 677269
You need to log in before you can comment on or make changes to this bug.