Reps: If a grip/object's property name is "more", it is clobbers the caption

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Shared Components
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: linclark, Assigned: Honza)

Tracking

Trunk
Firefox 52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [reserve-html])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Because Reps use the key "more" for captions, they will collide with any properties named more.

For example, if you have the following object:

> `{a: "a", b: "b", more: "c", d: "d", e: "e"}`

The output would be:

> `{a: "a", b: "b", more: "c"}`

But it should be:

> `{a: "a", b: "b", more: "c", more...}`
(Reporter)

Updated

2 years ago
Priority: -- → P1
Summary: If a grip/object's property name is "more", it is clobbers the caption → Reps: If a grip/object's property name is "more", it is clobbers the caption
Whiteboard: [devtools-html] [triage]
(Reporter)

Comment 1

2 years ago
Created attachment 8757279 [details] [diff] [review]
Bug1276206.patch

Removing the key is fine in this case. React will use its default, which is the array index of the Caption element. The "more..." caption should never be at a different position in the array, so using index as the key should have the same effect.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attachment #8757279 - Flags: review?(odvarko)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8757279 [details] [diff] [review]
Bug1276206.patch

Review of attachment 8757279 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

Honza
Attachment #8757279 - Flags: review?(odvarko) → review+
(Assignee)

Comment 3

2 years ago
Hm... I am now seeing:

"Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `Grip`. See https://fb.me/react-warning-keys for more information."


Honza

Updated

2 years ago
Blocks: 1257552, 1263741

Updated

2 years ago
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify-
Whiteboard: [devtools-html] [triage] → [devtools-html]
(Reporter)

Comment 4

2 years ago
Created attachment 8757319 [details] [diff] [review]
Bug1276206.patch

I realized that if we move the caption out of the array and just pass it in directly in the children arguments, as a sibling to props, then we won't get the warning.

Honza, what would you think of an approach like this? Not fully implemented yet, just wanted to get your feedback first.
Attachment #8757279 - Attachment is obsolete: true
Attachment #8757319 - Flags: feedback?(odvarko)
(Assignee)

Comment 5

2 years ago
(In reply to Lin Clark [:linclark] from comment #4)
> Created attachment 8757319 [details] [diff] [review]
> Bug1276206.patch
> 
> I realized that if we move the caption out of the array and just pass it in
> directly in the children arguments, as a sibling to props, then we won't get
> the warning.
> 
> Honza, what would you think of an approach like this? Not fully implemented
> yet, just wanted to get your feedback first.
Interesting, I didn't know keys work that way.
Anyway, works for me.

Honza
(Assignee)

Updated

2 years ago
Attachment #8757319 - Flags: feedback?(odvarko) → feedback+

Updated

2 years ago
Iteration: 49.3 - Jun 6 → 50.1
(Reporter)

Updated

2 years ago
Depends on: 1264685
(Reporter)

Updated

2 years ago
Blocks: 1276210

Updated

2 years ago
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.1 → ---
Priority: P1 → P2
I would like to work on this. Can Someone help me where to start?
(Reporter)

Comment 7

2 years ago
I have a patch up for this that I just have to apply a few changes to. Sorry that wasn't clearer, I didn't realize that my assignment to myself had been removed.

Another issue that would probably be good is this one: https://bugzilla.mozilla.org/show_bug.cgi?id=1264686
(Reporter)

Updated

2 years ago
Assignee: nobody → lclark
(In reply to Lin Clark [:linclark] from comment #7)
> I have a patch up for this that I just have to apply a few changes to. Sorry
> that wasn't clearer, I didn't realize that my assignment to myself had been
> removed.
> 
> Another issue that would probably be good is this one:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1264686
That depends on some other bug can you give me a good bug similar but free to take?
(Reporter)

Updated

2 years ago
No longer blocks: 1257552

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1

Updated

2 years ago
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29

Updated

2 years ago
Iteration: 51.2 - Aug 29 → ---
Priority: P1 → P3
Whiteboard: [devtools-html] → [reserve-html]

Updated

2 years ago
Status: ASSIGNED → NEW

Updated

2 years ago
Assignee: lclark → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Priority: P3 → P1
(Reporter)

Comment 9

2 years ago
Honza, if you're working on this be advised there's an easier way to handle this.

What you can do is use the rest syntax, e.g. ...values, to turn from an array to individual children. Then you don't need keys. As long as we aren't reordering any children, I think this is the best solution
(Assignee)

Comment 10

2 years ago
Created attachment 8795312 [details] [diff] [review]
bug1276206.patch

(In reply to Lin Clark [:linclark] from comment #9)
> Honza, if you're working on this be advised there's an easier way to handle
> this.
> 
> What you can do is use the rest syntax, e.g. ...values, to turn from an
> array to individual children. Then you don't need keys. As long as we aren't
> reordering any children, I think this is the best solution
Nice, I've done it that way.

- Object rep fixed + test updated.
- Grip rep fixed + test updated.
- Array/GripArray also changed, but just to make the code a bit more consistent. These reps don't have the problem (that's way there are no tests for these two).

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b96ea37cd4b

Honza
Attachment #8757319 - Attachment is obsolete: true
Attachment #8795312 - Flags: review?(lclark)

Updated

2 years ago
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
(Reporter)

Comment 11

2 years ago
Comment on attachment 8795312 [details] [diff] [review]
bug1276206.patch

Review of attachment 8795312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!
Attachment #8795312 - Flags: review?(lclark) → review+
(Assignee)

Comment 12

2 years ago
Thanks Lin!

Honza
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/327bd3387ff6
Avoid caption clobber if more prop exists. r=linclark
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/327bd3387ff6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
status-firefox49: affected → ---
You need to log in before you can comment on or make changes to this bug.